-
Notifications
You must be signed in to change notification settings - Fork 1.3k
api: Add params_show. #7613
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 params_show. #7613
Conversation
104090a to
b4766c9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
dvc/api.py
Outdated
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.
Wait, why? It’s a getter. Again, I am confused with porcelain/API discussion.
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.
@skshetry The argument during the last retro discussion was that this is at least following the CLI conventions. Could consider adding get_params as an alias though.
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.
Right, but is dvc.api the place to have these CLI wrappers?
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.
CLI wrappers?
What do you call CLI wrappers? I think the idea is that we follow CLI conventions regarding names, but this doesn't mean that the API is a CLI wrapper.
This API uses the internal Python API, raises exceptions, and returns a structure meant to be used in Python (instead of the internal structure returned by CLI --json options, for example).
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.
Personally, my vote would be get_params because the most immediate use case here is to onboard new users who aren't already familiar with the CLI.
However, I would rather get this merged than continue to discuss naming.
73bd12e to
333abb3
Compare
1 Could DVC detect a default stage based on the name of the file you're in? Then the no-args run would auto-filter that one stage's params specifically. Add 2 And maybe when loading a single stage's params, the JSON structure shouldn't have its name as key (so the code never needs to know it). E.g. # prepare.py
import json
import dvc.api
params = dvc.api.params_show()
print(json.dumps(params))
{
"split": 0.2,
"seed": 20170428
}
# use params['split'] instead of params[list(params.keys())[0]]['split']
|
dvc/api.py
Outdated
|
|
||
|
|
||
| def params_show( | ||
| *targets: 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.
Is it very relevant to pick by param files? Maybe the natural targets should be param names (return simple dict) or even stage names and this could be a secondary optional arg.
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.
Is it very relevant to pick by param files?
In all dvc {x} show / dvc {x} diff commands, targets are files, i.e. https://dvc.org/doc/command-reference/metrics/show
Maybe the natural targets should be param names (return simple dict)
Not sure I follow what this would look like.
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 agree that stage names are likely most useful here, but we are also trying to mimic the CLI. It's a good idea @jorgeorpinel but there has already been a lot of related discussions and I would rather get something merged than try to optimize the order of args.
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.
Makes sense to follow the CLI.
@jorgeorpinel We don't include the stage name, this is just how the example-get-started params are set up where there are sections inside the params file matching the name of the stage: |
This would be quite a big assumption to make and not sure how common users have this setup. |
dvc/repo/params/show.py
Outdated
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.
Hm, why do we need targets and stages? It it because targets should be paths? But what if you have stages with same names?
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.
Does stages here accept stage at a custom path? It appears it doesn't, unless I'm missing something.
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 do we need targets and stages?
Because they are not mutually exclusive. They have to be decoupled to support several scenarios, for example:
- A
stage(stage_A) uses params from differenttargets(target_A, target_B).
params_show(target_A, stage=stage_A)
- Different parts of a single
target(target_A) can be used for multiplestages(stage_A, stage_B).
params_show(target_A, stage=stage_B)
But what if you have stages with same names?
I am not sure I follow. The targets and stages filters are applied separately, so it would be up to the user to provide unambiguous arguments.
Does stages here accept stage at a custom path?
What's is a stage at a custom path?
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.
What's is a stage at a custom path?
Like path/to/dvc.yaml:mystage. Because mystage could be defined in multiple dvc.yaml at different levels.
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.
Like path/to/dvc.yaml:mystage. Because mystage could be defined in multiple dvc.yaml at different levels.
I updated to filter against stage.addressing instead of stage.name. The former includes the path/to/dvc.yaml: prefix when there are conflicts.
I tested with multiple dvc.yaml using same stage name and it works as expected when including the prefix in the argument
Defaults to `False`. If `True`, multiple `outs` sharing a provided `target_path` will not be filtered.
Closes #6507 Uses `repo.params.show` with custom error_handler and postprocess the outputs for more user-friendly structure. Extend `repo.params.show` to accept `stages` argument to cover the "params of current stage" use case.
Split into sub-modules. Split tests.
efiop
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.
Thank you @daavoo ! Let's run with this and change on top if we'll need anything.
Original PR #7613 by daavoo Original: treeverse/dvc#7613
Merged from original PR #7613 Original: treeverse/dvc#7613
Closes #6507
Uses
repo.params.showwith custom error_handler and postprocess the outputs for a more user-friendly structure.Extend
repo.params.showto acceptstagesargument to cover the "params of current stage" use case.dvc.orgP.R: treeverse/dvc.org#3459Examples
Will use the current project as
repoand retrieve all parameters,for all stages, for the current revision.
stages.repo.rev.