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

refactor(adaptive-core): improved readability recommendations #8431

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Vesyrak
Copy link

@Vesyrak Vesyrak commented Dec 28, 2023

This improves the readability and fixes a long-standing API issue in the adaptive_core

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

Copy link
Contributor

github-actions bot commented Dec 28, 2023

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

   27 files  ± 0     27 suites  ±0   1h 4m 3s ⏱️ + 3m 2s
3 486 tests ± 0  3 253 ✅  -  1  231 💤 ±0  1 ❌ +1   1 🔥 ±0 
7 217 runs  +10  6 233 ✅ +14  960 💤  - 6  2 ❌ +2  22 🔥 ±0 

For more details on these failures and errors, see this check.

Results for commit c3958d9. ± Comparison against base commit c98cd09.

♻️ This comment has been updated with latest results.

raise NotImplementedError()

async def recommendations(self, target: int) -> dict:
async def recommendations(self, target: int) -> Recommendation:
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a breaking change since it is possible to subclass Adaptive and all consumers of recommendations would break.

Copy link
Author

Choose a reason for hiding this comment

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

That's a very good point. I rewrote it into a TypedDict, so that it should remain backwards compatible while still improving readability.

@Vesyrak Vesyrak force-pushed the refactor/recommendations branch 4 times, most recently from 278cbac to 4c1bfea Compare January 12, 2024 13:28
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.

None yet

3 participants