Enforces specifying run_name and device_config_name on Engine interfaces #6649
Conversation
…sweep). Enforce requirement on Engine service.
senecameeks
requested review from
wcourtney,
vtomole,
cduck,
verult and
a team
as code owners
| @@ -64,8 +64,9 @@ async def run_async( | |||
| program_labels: Optional[Dict[str, str]] = None, | |||
| job_description: Optional[str] = None, | |||
| 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.
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. |
| @@ -56,6 +56,9 @@ class AbstractProcessor(abc.ABC): | |||
| 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
Removes default values and enforces specifying
run_nameanddevice_config_nameSpecial note that this does not disrupt the workflow