-
Notifications
You must be signed in to change notification settings - Fork 8
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
Changes needed to support execution via alchemiscale-fah
#218
Conversation
alchemiscale-fah
@ianmkenney can I get a review on this one? I'll keep it [WIP] for now as we finish things out over on OpenFreeEnergy/alchemiscale-fah#7, but getting a review sooner than later here will help us address clear gaps. |
count=count, | ||
protocols=protocols, | ||
) | ||
tasks = self._post_resource("/claim", data) | ||
|
||
return [ScopedKey.from_str(t) if t is not None else None for t in tasks] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment on the claim
endpoint. t
can't be None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is an oversight on my part. Ideally we'd like the API call to always given back a list of length count
, even if some elements must be None
. This is to mimic the behavior of the older claim_taskhub_tasks
method.
I've adjusted the return statement of the compute API claim_tasks
endpoint to reflect this.
@@ -187,13 +190,91 @@ def claim_taskhub_tasks( | |||
taskhub=taskhub_scoped_key, | |||
compute_service_id=ComputeServiceID(compute_service_id), | |||
count=count, | |||
protocols=protocols, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that n4js.claim_taskhub_tasks
has protocols typed as Protocol
s, not str
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your fixes in #279!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dotsdl added some comments. Do you know what's happening on the latest failed CI?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
==========================================
- Coverage 81.63% 80.23% -1.40%
==========================================
Files 26 27 +1
Lines 3371 3451 +80
==========================================
+ Hits 2752 2769 +17
- Misses 619 682 +63 ☔ View full report in Codecov by Sentry. |
* New protocols: DummyProtocolA, DummyProtocolB, DummyProtocolC * network_tyk2 now uses a variety of protocols for testing purposes
* Test if Tasks can be claimed by Protocol * Test if Tasks can be claimed by many Protocols
…-claim Test TaskHub Task claiming by Protocol
alchemiscale-fah
alchemiscale-fah
Co-authored-by: Ian Kenney <[email protected]>
Co-authored-by: Ian Kenney <[email protected]>
Thanks so much @ianmkenney! I'm going to merge; any additional changes made to support |
Includes all functionality needed for initial support of
alchemiscale-fah
, developed here: OpenFreeEnergy/alchemiscale-fah#7Also closes:
pydantic
models for encodingSynchronousComputeService
settings #107