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

params: show only params files that are actually used #5541

Closed
wants to merge 1 commit into from

Conversation

pared
Copy link
Contributor

@pared pared commented Mar 3, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Closes #5539

@pared pared requested a review from skshetry March 3, 2021 15:26
@pared pared force-pushed the 5451_default_params branch from 33d61f7 to fc8ae6e Compare March 3, 2021 15:30
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Looks good to me. But this default behaviour has been there since we introduced params, same as #5451 (introduced in #3631), so cc'ing @efiop if this was intended.

@pared pared requested a review from efiop March 4, 2021 09:05
@efiop
Copy link
Contributor

efiop commented Mar 8, 2021

It was intended originally, but now it is a mixup. Need to take a closer look and also look at the docs, as those might be affected. IIRC the logic was that people might use it without our pipelines (similar to same assumptions/discussions around metrics/plots). I don't feel comfortable just disabling this behavior, because it might turn out that it is needed in some scenario. We might need to double check params workflow first.

@skshetry
Copy link
Member

skshetry commented Mar 8, 2021

@efiop, isn't expecting a params.yaml file at the root of a git repository quite an assumption? πŸ™‚

There's --targets for this usecase already.

@efiop
Copy link
Contributor

efiop commented Mar 8, 2021

@skshetry Hard to tell. Params/metrics and plots are all filled with assumptions that were provided by users while developing it, and I also get constantly confused by it. The change seems logical, but we just need to make sure it fits into the workflow and we def need to check the docs.

@pared
Copy link
Contributor Author

pared commented Mar 8, 2021

@efiop

So the whole issue started in #5451, where user has quite extensive pipeline, and running exp show yielded unwanted results. This change was a prerequisite to make sure only used parameters are shown.

Params/metrics and plots are all filled with assumptions that were provided by users while developing it, and I also get constantly confused by it.

I guess we all are.
I think I jumped the horse here. The way to fix #5451 and not break just released 2.0.0 is to provide new flag that will allow user to show only used params, and keep this issue in mind when releasing 3.0. That way we keep the default behaviour.

@pared
Copy link
Contributor Author

pared commented Mar 11, 2021

Closing, we do not intend to change default behaviour for now.
Context:
#5451 (comment)

@pared pared closed this Mar 11, 2021
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.

params: what is the default params.yaml?
3 participants