Skip to content

don't allow loading stages without cmd #5373

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

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

skshetry
Copy link
Collaborator

@skshetry skshetry commented Feb 1, 2021

This was loosened when introducing parametrization.
But, foreach..do should be considered first
before the stage's regular structure. And, cmd is made
required (as it should have been)

Related: #5371, #5370, #5312

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

This was loosened when introducing parametrization.
But, `foreach`..`do` should be considered first
before stage's regular structure. And, cmd is made
required (as it should have been)

Related: iterative#5371, iterative#5370, iterative#5312
@skshetry skshetry added the bugfix fixes bug label Feb 1, 2021
@skshetry skshetry requested review from pmrowla, efiop and pared February 1, 2021 05:02
@skshetry skshetry self-assigned this Feb 1, 2021
@skshetry skshetry added the A: templating Related to the templating feature label Feb 1, 2021
Comment on lines +148 to +150
cmd = kwargs.get("cmd", "command")
stage = create_stage(
PipelineStage, dvc, path, name=name, cmd="", **kwargs
PipelineStage, dvc, path, name=name, cmd=cmd, **kwargs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll look into this on later iterations. It should never be empty.

@@ -6,7 +6,7 @@
from dvc.command.status import CmdDataStatus


def test_cloud_status(mocker):
def test_cloud_status(tmp_dir, dvc, mocker):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might need to move some of the logic in status command to API.

@skshetry skshetry merged commit 131eb7b into iterative:master Feb 1, 2021
@skshetry skshetry deleted the tighten-schema branch February 1, 2021 07:38
@pared
Copy link
Contributor

pared commented Feb 1, 2021

This PR contradicts the use case specified in here. I think its not common use case, but still we have been working on 4446 to make it work, @dmpetrov what is the desired behavior?

@skshetry
Copy link
Collaborator Author

skshetry commented Feb 2, 2021

@pared, I assumed "dummy stage" being a high-level concept. Could you please elaborate on that?
Do we need to support it in the dvc.yaml file?

@pared
Copy link
Contributor

pared commented Feb 2, 2021

@skshetry so the dummy stage was supposed to be an empty stage that would only contain params/metrics/plots so that one could trigger default behavior for dvc {params/plots/metrics} show/diff. I am not sure what is the particular use case but it was one of the requirements for #4446 - I guess we need some high-level perspective here.

@efiop
Copy link
Contributor

efiop commented Feb 2, 2021

@pared That's more of a workaround for our current inability to put metrics/etc into the dvc.yaml without creating a pipeline stage. If you remember, we were discussing something like:

data:
- data1
imports:
...
metrics:
- metric1
- metric2
plots:
...   
stages:
....

for the future. Do we need that workaround now? If we do, we could consider proper approach instead of bringing back a hack. CC @dberenbaum

@dberenbaum
Copy link
Contributor

Would metrics optionally have dependencies in the schema above?

@efiop
Copy link
Contributor

efiop commented Feb 2, 2021

Would metrics optionally have dependencies in the schema above?

@dberenbaum Not really. At least we didn't see any need for that yet. Theoretically the proposed schema could be extended with anything under specific metrics entry, but the scenario for declaring metric dependencies without a stage doesn't make sense. The only case where we handle such a thing is in imports as an edge case - if there is one dependency and one output and no command we assume that this is import.

@dberenbaum
Copy link
Contributor

I was assuming that all metrics would be defined at the top level, but it sounds like the idea is to support top-level metrics and metrics defined within stages?

@efiop
Copy link
Contributor

efiop commented Feb 2, 2021

@dberenbaum I was also thinking about that, but not as a new dag in top-level metrics, but more of a high-er level thing, where in stages there are only deps and outputs(e.g. metrics.json is an output) but then at the top level you also declare that this is not only an output but also a metric. But it seems like it is convenient to have support for both options: declare metrics inside of the stages and have an ability to declare them at the top level if they don't have their own stage or choose not to have it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: templating Related to the templating feature bugfix fixes bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants