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

Parse default remote properly #47

Closed
dmpetrov opened this issue Apr 13, 2020 · 7 comments · Fixed by #53
Closed

Parse default remote properly #47

dmpetrov opened this issue Apr 13, 2020 · 7 comments · Fixed by #53
Assignees

Comments

@dmpetrov
Copy link
Member

Currently, the remote type is checked in a not consistent way by finding string patterns like s3:// in dvc remote list outputs. That might be a problem when multiple remotes are defined.

In fact, dvc remote default returns the default remote name which can be properly resolved to URL in the dvc remote list output by a simple pattern.

Also, it makes sense to throw an error and exit with a proper message if dvc pull is required but the corresponded setting are not.

@DavidGOrtega
Copy link
Contributor

dvc-cml should not be handling this anymore, aside the exceptions like gs and ssh.
The code does nothing but warn so its removed in next PR delegating the responsibility to DVC.
In the case of gs and ssh it has to transform the json content inside the env variable into a file that gs is looking for. Something similar in ssh, the given key has to be added.

@dmpetrov
Copy link
Member Author

@DavidGOrtega you should not warn users if settings are not defined for a not default remote.

@dmpetrov
Copy link
Member Author

dmpetrov commented Apr 13, 2020

This is how you can get the default remote from bash

dvc remote list | grep "^`dvc remote default`\t" | awk '{print($2)}'

@DavidGOrtega
Copy link
Contributor

Thats what Im saying. DVC-CML only needs to interface the secrets to the runner in case it has to do it.
Right now that only happens in gs and ssh.
For the rest the whole code is removed delegating it to dvc

@dmpetrov
Copy link
Member Author

Log:

Fetch all history for all tags and branches
Setting DVC remote ...
S3 DVC remote found but no credentials found
Pulling from DVC remote ...
Failed pulling from DVC remote
Error: Command failed: dvc pull -f
Running dvc repro Dvcfile
ERROR: unexpected error - Unable to locate credentials

    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:310:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Socket.<anonymous> (internal/child_process.js:443:11)
    at Socket.emit (events.js:310:20)
    at Pipe.<anonymous> (net.js:672:12) {
  killed: false,
  code: 255,
  signal: null,
  cmd: 'dvc pull -f'
}
##[error]Command failed: dvc repro Dvcfile            <--- THIS IS IN RED
WARNING: Dependency 'train.py' of 'Dvcfile' changed because it is 'modified'.
WARNING: Stage 'Dvcfile' changed.
.... [CONTINUE SOME PROCESS]

How the log ideally should look like:

Fetch all history for all tags and branches
Setting DVC remote ...
S3 DVC remote found but no credentials found
Pulling from DVC remote ...
Failed pulling from DVC remote
Error: Command failed: dvc pull -f
Running dvc repro Dvcfile
ERROR: unexpected error - Unable to locate credentials   <--- THIS IS IN RED
[EXIT NOW - NO REASON TO CONTINUE]
##[error]Process completed with exit code 1.               <--- THIS IS IN RED 

@DavidGOrtega
Copy link
Contributor

[EXIT NOW - NO REASON TO CONTINUE]
Its something that you have added right?

@dmpetrov
Copy link
Member Author

@DavidGOrtega yes

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 a pull request may close this issue.

2 participants