update: default no longer returning RETIRED devices from get_devices #796
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #796 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 129 129
Lines 8372 8374 +2
Branches 1865 1866 +1
=========================================
+ Hits 8372 8374 +2 ☔ View full report in Codecov by Sentry. |
src/braket/aws/aws_device.py
Outdated
| statuses (Optional[list[str]]): device status list, default is `None`. When `None` | ||
| is used, RETIRED devices will not be returned. To include RETIRED devices in | ||
| the results, add `RETIRED` to the list passed to this parameter. |
There was a problem hiding this comment.
We typically state the default semantically used, even if the function signature has =None. Here, I believe the default is ["ONLINE", "OFFLINE"]. We can add a note in the doc string that the default behavior is to filter out "RETIRED"
There was a problem hiding this comment.
I am generally not a fan of listing this stuff out here since it can more easily go out-of-date (like if we add another state). Then, for users of older versions of the BDK, it would actually be wrong as it would return more than those two.
There was a problem hiding this comment.
The docs are versioned by release, so I'm not sure I see the issue. The doc string is wrong for older versions of the bdk regardless; I think it's just a question of the phrasing here
There was a problem hiding this comment.
Oh I see what you mean by listing stuff out. Would it be better to just add a new parameter filter_retired=True that kicks in when statuses is None to separate the logic?
There was a problem hiding this comment.
I considered it, butI think it would be confusing to add a parameter that can have a direct opposite effect of another parameter (ie. have a filter_retired=True and statuses=["RETIRED"])
There was a problem hiding this comment.
I see that "default is None" is consistent with the other doc strings. I don't love how the argument doc strings don't actually specify the default behavior, just the default value of the parameter.
I'd prefer to see these called out as filters explicitly, maybe even just changing "device name list, default is None" to "device name filter, default is None". Then we could clarify statuses like "device status filter, default is None. When None is used, RETIRED devices will not be returned. To include RETIRED devices in the results, use a filter including "RETIRED" for this parameter."
| if statuses is None and result["deviceStatus"] == "RETIRED": | ||
| continue |
There was a problem hiding this comment.
Am I missing something, or does this not actually impact the search_devices call? Does this fix our time issue? It looks like it still gets every device but then filters after; is this a limitation of the search devices api?
There was a problem hiding this comment.
It does fix our time issue. Even though the API returns all values, we will filter our the retired ones. This means that when we actually do get_device for all returned devices, we will be doing many fewer calls.
There was a problem hiding this comment.
Ah that's the piece I was missing, that makes sense if get_device is what's taking a long time
Done. |
src/braket/aws/aws_device.py
Outdated
| provider_names (Optional[list[str]]): provider name list, default is `None` | ||
| statuses (Optional[list[str]]): device status filter, default is `None`. When `None` | ||
| is used, RETIRED devices will not be returned. To include RETIRED devices in | ||
| the results, use a filter that includes `RETIRED` for this parameter. |
There was a problem hiding this comment.
Smallest nit: in this context we mean the string "RETIRED", unless we have an enum somewhere defining RETIRED
Issue #, if available:
#448
Description of changes:
Testing done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.