enable loading config.yml files from URL #3977

ltalirz · 2020-04-28T07:52:03Z

In the view of the plans of starting online repositories that maintain
yaml configuration files for computers and codes, it becomes very useful
to be able to directly import a configuration file from a URL.

Todo:

write a test

unkcpz

hi @ltalirz ,

First of all, thanks for letting me participate in the code review. I can really learn a lot from it. I made some specific change requests below.
One thing need to be discussed is whether it is possible to provide a valid url somewhere and add test cases for actually test UrlPath and UrlFile?

aiida/cmdline/params/types/__init__.py

aiida/cmdline/params/options/__init__.py

tests/cmdline/commands/test_import.py

aiida/cmdline/params/types/path.py

ltalirz · 2020-06-02T07:46:24Z

Thanks a lot for your review @unkcpz !

One thing need to be discussed is whether it is possible to provide a valid url somewhere and add test cases for actually test UrlPath and UrlFile?

You are absolutely correct.
I had actually added this test but somehow I managed to mess up what was pushed to github and the code got lost (I'm still puzzled as to what I did..).
So, let's just say that the missing test was an exercise for you as a reviewer and that you passed the test ;-)
I have added it back again and addressed the other issues you pointed out.

P.S. Don't worry about the failing test of circleci

docs/source/verdi/verdi_user_guide.rst

.pre-commit-config.yaml

unkcpz · 2020-06-02T09:36:07Z

Thanks a lot @ltalirz , just few questions from my personal side.

You haven't put any more traps in it to test me, have you? 😆

ltalirz · 2020-06-02T10:32:41Z

@unkcpz Please have another look and review again - pushing changes dismisses a previous approving review (just in case this was not clear)

unkcpz

It seems all fine. Thanks! @ltalirz

ltalirz · 2020-06-02T13:34:34Z

@chrisjsewell @sphuber The circleci tests are failing and blocking me from merging this PR.
Are they to be taken seriously?

ltalirz · 2020-06-02T13:35:05Z

Ah no, sorry - it seems @unkcpz's review does not yet "count". Will fix this :-)

ltalirz · 2020-06-02T13:46:43Z

@chrisjsewell @sphuber
While Jason's review is now counted, I still can't merge this PR because of the failing CircleCI test (ci/circleci: docs is required to pass in the branch protection rules).
So I'm back to my original question: Are these tests to be taken seriously? If not, why are they required to pass?
The test failure seems to be independent of this PR.

chrisjsewell · 2020-06-02T13:48:39Z

@ltalirz there is no config.yaml available for circleci until #4141 is merged

sphuber · 2020-06-02T13:56:33Z

I am in the process of merging the docs revamp now. Please hold off with this PR until then. You can then rebase and the tests should then automatically start running

codecov · 2020-06-02T15:19:18Z

Codecov Report

Merging #3977 into develop will decrease coverage by 0.01%.
The diff coverage is 94.29%.

@@             Coverage Diff             @@
##           develop    #3977      +/-   ##
===========================================
- Coverage    78.75%   78.74%   -0.00%     
===========================================
  Files          467      467              
  Lines        34454    34466      +12     
===========================================
+ Hits         27130    27138       +8     
- Misses        7324     7328       +4

Flag	Coverage Δ
#django	`70.67% <94.29%> (-<0.01%)`	⬇️
#sqlalchemy	`71.54% <94.29%> (-<0.01%)`	⬇️

Impacted Files	Coverage Δ
aiida/cmdline/params/types/path.py	`90.77% <93.34%> (-1.53%)`	⬇️
aiida/cmdline/commands/cmd_import.py	`77.50% <100.00%> (ø)`
aiida/cmdline/params/options/config.py	`100.00% <100.00%> (ø)`
aiida/common/extendeddicts.py	`93.62% <100.00%> (ø)`
aiida/engine/daemon/client.py	`72.58% <0.00%> (-1.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a427699...56dabd0. Read the comment docs.

sphuber · 2020-06-02T15:31:15Z

I put this on hold because I am also going through a review, so please don't merge this yet

sphuber

Thanks @ltalirz

tests/cmdline/params/types/test_path.py

sphuber · 2020-06-02T15:51:35Z

aiida/cmdline/commands/cmd_import.py

@@ -17,7 +17,7 @@

 from aiida.cmdline.commands.cmd_verdi import verdi
 from aiida.cmdline.params import options
-from aiida.cmdline.params.types import GroupParamType, ImportPath
+from aiida.cmdline.params.types import GroupParamType, PathURL


Two points about the name:

I did not immediately understand that this type basically allows a 'Path' or 'URL'. But that is the only reason for its existence, since we have options that need to support both and Click does not support parameters of multiple type. So I think perhaps the name PathOrUrl and FileOrUrl would make this a lot easier to see.

Second point is already made implicitly in the previous. It is a matter of convention, but both Microsoft and Google advocate to stick to camel-case (or pascal-case) rules, even when it comes to acronyms. So URL should become Url.

Yeah... I guess what you would really want is to be able to pass multiple types, but that's not how the click API is built.
Besides concatenating Class names with Or, one can also just choose one of them (e.g. File). In that case, one then has to check the documentation to know exactly how our File implementation differs from the standard one.
Anyhow, I've used your suggestions for now. Just not entirely sure about the FILEORURL metavar... I guess it's good enough for the moment.

In the view of the plans of starting online repositories that maintain yaml configuration files for computers and codes, it becomes very useful to be able to directly import a configuration file from a URL. Version 0.6 of the config-file-option now respects the `type` parameter, and file handles can be passed directly to the configuration file provider. This allows handling opening files locally and from URLs on the same footing.

ltalirz · 2020-06-02T17:19:56Z

Very strange pre-commit failure https://github.com/aiidateam/aiida-core/pull/3977/checks?check_run_id=731694434

Anyhow, I'm silencing the pre-commit warning now

sphuber · 2020-06-02T18:05:47Z

Thanks @ltalirz and @unkcpz !

ltalirz marked this pull request as draft April 28, 2020 07:52

ltalirz force-pushed the config-from-url branch 6 times, most recently from 9bf0c03 to aa3c2ee Compare April 28, 2020 09:39

ltalirz force-pushed the config-from-url branch 2 times, most recently from bf6ea84 to 5c29bfc Compare June 1, 2020 23:20

ltalirz requested a review from unkcpz June 1, 2020 23:20

ltalirz marked this pull request as ready for review June 1, 2020 23:21

unkcpz requested changes Jun 2, 2020

View reviewed changes

aiida/cmdline/params/types/path.py Outdated Show resolved Hide resolved

ltalirz force-pushed the config-from-url branch 2 times, most recently from 923c7c6 to 0c4137b Compare June 2, 2020 07:43

ltalirz requested a review from unkcpz June 2, 2020 07:47

ltalirz force-pushed the config-from-url branch 2 times, most recently from 263f93b to 801fe59 Compare June 2, 2020 07:50

unkcpz reviewed Jun 2, 2020

View reviewed changes

docs/source/verdi/verdi_user_guide.rst Outdated Show resolved Hide resolved

.pre-commit-config.yaml Show resolved Hide resolved

ltalirz force-pushed the config-from-url branch from 801fe59 to b4554a0 Compare June 2, 2020 09:44

ltalirz requested a review from unkcpz June 2, 2020 09:48

unkcpz previously approved these changes Jun 2, 2020

View reviewed changes

ltalirz dismissed unkcpz’s stale review via 9e605fa June 2, 2020 15:00

ltalirz force-pushed the config-from-url branch from b4554a0 to 9e605fa Compare June 2, 2020 15:00

sphuber added the pr/on-hold PR should not be merged label Jun 2, 2020

sphuber requested changes Jun 2, 2020

View reviewed changes

ltalirz force-pushed the config-from-url branch from 9e605fa to 7ef519d Compare June 2, 2020 16:13

ltalirz requested a review from sphuber June 2, 2020 16:15

ltalirz force-pushed the config-from-url branch from 7ef519d to bceff08 Compare June 2, 2020 16:36

ltalirz force-pushed the config-from-url branch from bceff08 to 56dabd0 Compare June 2, 2020 17:19

sphuber approved these changes Jun 2, 2020

View reviewed changes

sphuber merged commit e9a3e95 into aiidateam:develop Jun 2, 2020

sphuber deleted the config-from-url branch June 2, 2020 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable loading config.yml files from URL #3977

enable loading config.yml files from URL #3977

ltalirz commented Apr 28, 2020 •

edited

Loading

unkcpz left a comment

ltalirz commented Jun 2, 2020

unkcpz commented Jun 2, 2020

ltalirz commented Jun 2, 2020 •

edited

Loading

unkcpz left a comment

ltalirz commented Jun 2, 2020

ltalirz commented Jun 2, 2020

ltalirz commented Jun 2, 2020

chrisjsewell commented Jun 2, 2020

sphuber commented Jun 2, 2020

codecov bot commented Jun 2, 2020 •

edited

Loading

sphuber commented Jun 2, 2020

sphuber left a comment

sphuber Jun 2, 2020

ltalirz Jun 2, 2020

ltalirz commented Jun 2, 2020

sphuber commented Jun 2, 2020

enable loading config.yml files from URL #3977

enable loading config.yml files from URL #3977

Conversation

ltalirz commented Apr 28, 2020 • edited Loading

unkcpz left a comment

Choose a reason for hiding this comment

ltalirz commented Jun 2, 2020

unkcpz commented Jun 2, 2020

ltalirz commented Jun 2, 2020 • edited Loading

unkcpz left a comment

Choose a reason for hiding this comment

ltalirz commented Jun 2, 2020

ltalirz commented Jun 2, 2020

ltalirz commented Jun 2, 2020

chrisjsewell commented Jun 2, 2020

sphuber commented Jun 2, 2020

codecov bot commented Jun 2, 2020 • edited Loading

Codecov Report

sphuber commented Jun 2, 2020

sphuber left a comment

Choose a reason for hiding this comment

sphuber Jun 2, 2020

Choose a reason for hiding this comment

ltalirz Jun 2, 2020

Choose a reason for hiding this comment

ltalirz commented Jun 2, 2020

sphuber commented Jun 2, 2020

ltalirz commented Apr 28, 2020 •

edited

Loading

ltalirz commented Jun 2, 2020 •

edited

Loading

codecov bot commented Jun 2, 2020 •

edited

Loading