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

addressing: allow stage name addressing without ":" #3842

Merged
merged 9 commits into from May 26, 2020

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented May 21, 2020

This PR enables users to run/work with stage via their name if they are in dvc.yaml file.

Example

$ dvc commit stage_name

There is a caveat though: what if the output and the stage name clashes?
Currently, there is no way to specify it is an output, but, there is way to indicate it's a stage via:

$ dvc commit :stage_name  # or,
$ dvc commit dvc.yaml:stage_name

The only good workaround is:

$ dvc checkout ./output-filename-that-clashes-with-stage-name
  • ❗ I have followed the Contributing to DVC checklist.

  • 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

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

@skshetry skshetry self-assigned this May 21, 2020
@skshetry skshetry changed the title addressing: allow stage name addressing without ":" [WIP] addressing: allow stage name addressing without ":" May 21, 2020
@efiop efiop moved this from In progress to Review in progress in DVC Sprint 19 May - 2 June 2019 May 22, 2020
@skshetry
Copy link
Member Author

skshetry commented May 22, 2020

Looks like we'll have a problem when we have a output and stage of same name. At the moment, the priority is for stage name.

I don't want to provide priority to outputs, as it really slows things down. Currently, there's no way to indicate that the file is an output, not a stage in case of collision.

I know, it can happen very rarely, but it is still a problem that should be solved.

$ ls
copy-foo-bar copy-foo-bar.dvc dvc.yaml dvc.lock
$ # dvc.yaml has stage with name `copy-foo-bar`
$ dvc checkout copy-foo-bar
A    bar

The workaround for now is:

$ dvc checkout ./copy-foo-bar

cc @efiop @shcheklein

@shcheklein
Copy link
Member

The workaround for now is:

this is a good and clear workaround to my mind!

@skshetry skshetry changed the title [WIP] addressing: allow stage name addressing without ":" addressing: allow stage name addressing without ":" May 25, 2020
@skshetry skshetry marked this pull request as ready for review May 25, 2020 13:40
@skshetry skshetry requested review from efiop, pared and pmrowla May 25, 2020 13:41
@skshetry
Copy link
Member Author

The workaround for now is:

this is a good and clear workaround to my mind!

Clear: yes, but workaround nonetheless. We are making a distinction
between ./copy-foo-bar and copy-foo-bar which I don't feel good about.

But, as it should happen rarely, and in docs, we should suggest users to not
clash stage name with folder name (to make their life easier for -R) and
output file/folder name.

stats[key].extend(
_fspath_dir(path, self.root_dir) for path in items
)
stats[key].extend(_fspath_dir(path) for path in items)
Copy link
Member Author

Choose a reason for hiding this comment

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

Before, the checkout summary were shown as path_in_repo. This should now actually show in terms of relpath.

Also remove `cause` when throwing NoOutputOrStage, as
CheckoutErrorSuggestGit will start chaining all the messages
in cause of the exception.
dvc/exceptions.py Outdated Show resolved Hide resolved
@efiop
Copy link
Member

efiop commented May 25, 2020

Clear: yes, but workaround nonetheless. We are making a distinction
between ./copy-foo-bar and copy-foo-bar which I don't feel good about.

@skshetry That is genuinely a very good approach, which is also intuitive, kinda like with launching scripts on *nix. I really like it personally.

@efiop efiop merged commit 2fecc66 into iterative:master May 26, 2020
DVC Sprint 19 May - 2 June 2019 automation moved this from Review in progress to Done May 26, 2020
Comment on lines 317 to 319
raise NoOutputOrStage(target, exc.file)
raise NoOutputOrStageError(target, exc.file) from exc
except StageNotFound as exc:
raise NoOutputOrStage(target, exc.file)
raise NoOutputOrStageError(target, exc.file) from exc
Copy link
Member Author

Choose a reason for hiding this comment

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

We concat message of every chain of exceptions. So, when we get CheckoutErrorSuggestGit,
we will get following error message:

$ dvc repro random_name
ERROR: Did you mean `git checkout random_name`?: 'random_name' does not exist as an output or a stage name in 'dvc.yaml': "Stage 'random_name' not found inside 'dvc.yaml' file"

So, I left it out, as context is anyway set if they are in try...except block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternative thing to do is only concat messages from 2 exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants