-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: SDK methods to fetch pachyderm configs [MD-406] #9348
Conversation
✅ Deploy Preview for determined-ui canceled.
|
e89b6a8
to
0fe9e05
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9348 +/- ##
==========================================
- Coverage 45.16% 45.11% -0.06%
==========================================
Files 1230 1229 -1
Lines 154523 154524 +1
Branches 2404 2404
==========================================
- Hits 69788 69707 -81
- Misses 84540 84622 +82
Partials 195 195
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -449,6 +449,23 @@ def get_metrics(self, group: Optional[str] = None) -> Iterable["metrics.TrialMet | |||
for d in resp.metrics: | |||
yield metrics.TrialMetrics._from_bindings(d, group) | |||
|
|||
def get_pachyderm_commit(self) -> 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.
non-blocking question: Is there a reason why we're exposing the commit hash specifically? Is there a use case for retrieving this field that's different than the other fields in the pachyderm configuration?
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.
yeah, it's mainly a product framing consideration. we want to make the integration seem smooth.
conceptually we're putting pachyderm integration on a determined checkpoint assuming the user wants to get the data associated with this checkpoint. my understanding is that the commit hash is the only thing you need to be able to get the data in pachyderm, so the idea here is to make the user journey between determined checkpoints and its pachyderm data obvious. it'd be weird if a determined checkpoint object returned the whole pachyderm config.
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.
Would it be possible to automate the test plan?
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.
Looks good!
so, this could be made into an e2e test, but i'm not sure it's worth the additional cost to run. the change itself is rather small and logically trivial IMO. most of the existing expconf logic is tested elsewhere, this change just grabs a key out of it. i had originally written unit/mock tests for these methods, but decided not to include them because mocking the API endpoints reduced the logic down to "pass in a mocked response dict, make sure the exact same dict is returned", and it felt like a silly test to include. if we build out this pachyderm integration further to include logic beyond getting a key from a dict, then i think tests would definitely be warranted. |
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.
Works as expected
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.
LGTM!
Ticket
Description
We want to expose pachyderm configs from the expconf for pachyderm integrations in the python SDK. These are basically just convenience methods for getting a specific field out of the experiment config. Pachyderm configs were added to the expconf in #8933 and take the following structure:
these are expected to be populated mostly manually by users today
Test Plan
integrations: pachyderm
config defined in the expconf, and b) creates at least 1 checkpoint.For example, using the Core API script at
examples/tutorials/core_api/2_checkpoints.py
, with the following expconf:pach.yaml
the commit and configs that are printed should match what was submitted in the experiment config.
Checklist
docs/release-notes/
.See Release Note for details.