-
Notifications
You must be signed in to change notification settings - Fork 1.3k
api: Add exp_show.
#9356
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
api: Add exp_show.
#9356
Conversation
7631a01 to
f29ef35
Compare
|
Should To return the native python types it would just be I was also wondering if supporting the |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9356 +/- ##
=======================================
Coverage 92.60% 92.60%
=======================================
Files 461 463 +2
Lines 37322 37378 +56
Branches 5380 5388 +8
=======================================
+ Hits 34561 34615 +54
- Misses 2207 2209 +2
Partials 554 554
☔ View full report in Codecov by Sentry. |
I feel that from the user's perspective the current result from
On this, I am also unsure. Assuming the all_exps = []
for rev in revs:
all_exps.extend(exp_show(rev=rev))It would be nice if we complement this with some pubic helpers for getting those common sets of revisions |
79d71d0 to
36970c0
Compare
Agreed. I think we had similar conversations around
Not a blocker, but IMO an arg that accepts multiple revs makes more sense since I think it's likely to be a much more common operation than for |
But what about the |
I think it's fine to drop them if we have [edit: tbh I still think they would be helpful unless we plan to provide a bunch of example on how to iterate over revisions (like getting all branches or all commits on a branch), but not a blocker.] |
36970c0 to
0ccf532
Compare
Pushed an update accepting only |
0ccf532 to
852ae09
Compare
852ae09 to
0c9699e
Compare
dberenbaum
left a comment
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.
I haven't done QA, but the interface LGTM!
0c9699e to
28e3a0b
Compare
|
@daavoo Can we make an issue to add this to all the example notebooks where we currently use edit: also a docs pr 🙏 |
| hide_queued (bool, optional): hide experiments that are queued for | ||
| execution. | ||
| Defaults to `False`. | ||
| hide_failed (bool, optional): hide experiments that have failed. | ||
| sha (bool, optional): show the Git commit SHAs of the experiments | ||
| instead of branch, tag, or experiment names. | ||
| Defaults to `False`. | ||
| param_deps (bool, optional): include only parameters that are stage | ||
| dependencies. | ||
| Defaults to `False`. |
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.
Sorry @daavoo, I should have reviewed this closer before merging. Nothing severe, but I have minor questions when you have a chance:
- Why did you choose to include these options to hide rows or columns but not others like
only-changed? - What's the point of including
shasince there's already arevcolumn?
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.
Why did you choose to include these options to hide rows or columns but not others like only-changed?
Because these are applied at the "low level" collection and the others are applied on the UI. I will open P.R. dropping them, though
What's the point of including sha since there's already a rev column?
There is no point.
* api-reference: Add `exp_show` and `scm`. Per treeverse/dvc#9356 * Update content/docs/api-reference/exp_show.md Co-authored-by: Dave Berenbaum <dave@iterative.ai> * Add exp_show to sidebar * updates * Add example to scm * fix * Update content/docs/api-reference/exp_show.md --------- Co-authored-by: Dave Berenbaum <dave@iterative.ai>
Closes #8775
Closes #7167
Using
scmhelpers:TODO: