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

Add `dvc version` command #1963

Merged
merged 6 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@algomaster99
Copy link
Contributor

commented May 2, 2019

  • Have you followed the guidelines in our
    Contributing document?

  • Does your PR affect documented changes or does it add new functionality
    that should be documented? If yes, have you created a PR for
    dvc.org documenting it or at
    least opened an issue for it? If so, please add a link to it.


I have added the dvc version command which will aim to output the system/environment information along with the DVC version. I would like some suggestions on what all information might be required.

Current Output

Screenshot from 2019-05-03 01-23-28

Referring to issue #1783, Cache information seemed necessary, I will add that also once I figure that out. Writing tests also remain.

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Can anyone review this work-in-progress pull request?

Show resolved Hide resolved dvc/command/version.py Outdated
@efiop
Copy link
Member

left a comment

Looks good! 🎉 Let's also not forget to add a test for this new command 🙂 See tests/func/.

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@efiop I have changed the import such that it uses __version__. I will write tests and push it in another commit.

@efiop

This comment has been minimized.

Copy link
Member

commented May 3, 2019

For the record: take a look at https://github.com/iterative/dvc/blob/master/tests/func/test_metrics.py#L283 and how it captures info output and compares it with expected output.

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@efiop Okay, thanks for the information. I will make the changes and push them as soon as possible.

@efiop

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Update: here is a more suitable example that is using pytest's fixtures https://github.com/iterative/dvc/blob/master/tests/func/test_metrics.py#L810

@algomaster99 algomaster99 force-pushed the algomaster99:dvc_version branch from a23ff2c to 2138895 May 10, 2019

@shcheklein
Copy link
Member

left a comment

This is looks great. Thanks! 🎉 If it easy enough let's add at least a file system name, which link type it supports - hardlinks, reflinks, etc.

@mroutis
Copy link
Collaborator

left a comment

Left some comments, @algomaster99

Show resolved Hide resolved tests/func/test_version.py Outdated
Show resolved Hide resolved tests/func/test_version.py Outdated
Show resolved Hide resolved tests/func/test_version.py Outdated
Show resolved Hide resolved dvc/command/version.py Outdated

@algomaster99 algomaster99 changed the title [WIP] Add `dvc version` command Add `dvc version` command May 10, 2019

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@mroutis I have made the changes. Please review it.
@shcheklein I will add that info in the next commit.

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@shcheklein I am not able to find a python function which displays the filesystem and links info.

@mroutis

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

@shcheklein I am not able to find a python function which displays the filesystem and links info.
@algomaster99 , the links information is gathered from DVC's config file.

For the file system in use is a little bit more complicated, maybe we can skip this for the sake of merging the PR, what do you think, @shcheklein ?

https://stackoverflow.com/questions/25283882/determining-the-filesystem-type-from-a-path-in-python

@mroutis

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Also, @efiop , @algomaster99 , @shcheklein , what do you think about changing the name from version to info as I suggested in the issue?

I find useful to add configured remotes and environment information just to know what we are dealing with debugging. (only if it easy and feasible)

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@mroutis Okay, I can rename the command from version to info. But are we doing this to resemble the docker info command?

Show resolved Hide resolved dvc/command/version.py
Show resolved Hide resolved tests/func/test_version.py Outdated
Show resolved Hide resolved tests/func/test_version.py Outdated
Show resolved Hide resolved tests/func/test_version.py Outdated
@mroutis

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

@algomaster99 , the config object should be available in the self of your CmdVersion class:

>>> self.config.config
ConfigObj({'remote "dvc"': {'url': 'https://dvc.org/s3/dvc', '_cwd': '/home/mroutis/src/iterative/dvc/.dvc', 'use_ssl': True, 'listobjects': Fa
lse, 'protected': False}, 'core': {'remote': 'dvc', 'storagepath': '', 'cloud': '', 'analytics': True, 'interactive': False}, 'aws': {}, 'state
': {}, 'gcp': {}, 'cache': {}, 'local': {}})
@shcheklein

This comment has been minimized.

Copy link
Member

commented May 10, 2019

For the file system in use is a little bit more complicated, maybe we can skip this for the sake of merging the PR, what do you think, @shcheklein ?

@mroutis @algomaster99 Yep, agreed. Let's skip it and create a separate ticket to add it.

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@mroutis

  • Moved the test under tests/unit/command
  • Modify regex pattern like you suggested
  • Used caplog.text since a single record is there

Any other changes?

@algomaster99

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@mroutis Do I rename the command as well?

@shcheklein

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@mroutis Do I rename the command as well?

I actually don't like the idea of renaming. Potentially we will be able to find a better use case for the dvc info, something related to the core functionality. Also, it's pretty common to use version to output environment information.

@shcheklein
Copy link
Member

left a comment

Great stuff @algomaster99 ! Thanks!

@mroutis mroutis requested a review from efiop May 10, 2019

@mroutis

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

great, @shcheklein , let's roll with version then

changes were made :)

@efiop

efiop approved these changes May 10, 2019

Copy link
Member

left a comment

Perfect, thank you! 🎉

@efiop efiop merged commit 45f94e3 into iterative:master May 10, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@algomaster99 algomaster99 deleted the algomaster99:dvc_version branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.