Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Device Capabilities] Add capabilities to the device API (WIP) #6433

Open
wants to merge 5 commits into
base: device-cap
Choose a base branch
from

Conversation

astralcai
Copy link
Contributor

Follow up PR to #6407, adds capabilities to the device API.

[sc-71716]
[sc-71709]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.38%. Comparing base (fffafe2) to head (b7e81c6).

Additional details and impacted files
@@             Coverage Diff             @@
##           device-cap    #6433   +/-   ##
===========================================
  Coverage       99.38%   99.38%           
===========================================
  Files             448      448           
  Lines           42560    42576   +16     
===========================================
+ Hits            42300    42316   +16     
  Misses            260      260           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +187 to +194
def capabilities(self) -> dict:
"""To override the Device class's capabilities property

TODO: unify the behaviour of the old `capabilities` and the new.

"""
return self._device.capabilities()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we can map the child capabilities to a DeviceCapabiltiies class, we should probably just return None here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does any legacy device still depends on its capabilities being a method that returns a dictionary? Many of our tests currently do, e.g.,

capabilities = dev.capabilities().copy()

Should we update the tests to get rid of its dependency on the capabilities method?

@@ -121,6 +123,25 @@ class Device(abc.ABC):

"""

config: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think of config_location instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about config_file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants