Enforces specifying run_name and device_config_name on Engine interfaces#6649
Conversation
…sweep). Enforce requirement on Engine service.
senecameeks
requested review from
a team,
cduck,
verult,
vtomole and
wcourtney
as code owners
NoureldinYosri
left a comment
NoureldinYosri
left a comment
There was a problem hiding this comment.
Shouldn't this PR have the BREAKING CHANGE tag?
| job_labels: Optional[Dict[str, str]] = None, | ||
| run_name: str = "", | ||
| device_config_name: str = "", | ||
| *, |
There was a problem hiding this comment.
do we need the * here and elsewhere? what is the purpose?
There was a problem hiding this comment.
Mainly for readability and convenience. This preserves parameter ordering and keeps the number of required changes low.
There was a problem hiding this comment.
On a second look, it is a bit strange to have optional positional arguments followed by mandatory kw arguments run_name, device_config_name. I tried to move the added *, just before the first optional arguments and the tests pass.
Can we do so and also move the run_name, device_name as the first arguments after *,?
The order of kw-only arguments does not matter, but for code readability it is nicer to have
mandatory arguments followed by optional.
There was a problem hiding this comment.
Good point! Moved.
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with a small change to docstrings.
senecameeks
added
the
BREAKING CHANGE
|
@NoureldinYosri added |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6649 +/- ##
=======================================
Coverage 97.81% 97.81%
=======================================
Files 1066 1066
Lines 91779 91779
=======================================
Hits 89773 89773
Misses 2006 2006 ☔ View full report in Codecov by Sentry. |
| async def run_async( | ||
| self, | ||
| program: cirq.Circuit, | ||
| *, |
There was a problem hiding this comment.
nit: try to remove the * and see what breaks. if a few things break then we can fix them
There was a problem hiding this comment.
nit: try to remove the
*and see what breaks. if a few things break then we can fix them
This may be dangerous, because the main-branch version had 2 optional string arguments at those positions.
Requiring everything to be a kwarg will make sure that string arguments are not mixed up.
|
@senecameeks - please wait for #6650 to make it in and then merge it here. |
…ces (quantumlib#6649) * Remove default values from client interfaces (e.g. EngineProgram.run_sweep). Enforce requirement on Engine service. * fix * more test files changes * address nits * mv args * lint
4 participants
Removes default values and enforces specifying
run_nameanddevice_config_nameSpecial note that this does not disrupt the workflow