Add verdi code duplicate #1737

ltalirz · 2018-07-10T13:07:43Z

Still to do:

add option to hide previous code
add tests
think about whether it makes sense to avoid duplication of click parameters for verdi code duplicate and verdi code setup

Still to do: * add possibility to override parameters * store duplicated code * add tests

still to do * add option to hide previous code * add tests

ltalirz · 2018-07-14T14:49:07Z

@sphuber The tests fail when "submitting 15 old-style calculations to the daemon", and the error is

aiida.common.exceptions.MissingEntryPointError: Entry point 'simpleplugins.templatereplacer = aiida.orm.calculation.job.simpleplugins.templatereplacer:TemplatereplacerCalculation' not found in group 'aiida.calculations'

https://travis-ci.org/ltalirz/aiida_core/jobs/403912242#L1540

This is unexpected, since I haven't modified any calculation-related files.
Also, when I do verdi calculation plugins, the simpleplugins.templatereplacer plugin still shows up.

Is the reason for the error that it tries to find the string simpleplugins.templatereplacer = ... instead of just simpleplugins.templatereplacer?

If you don't have any pointers, I'll investigate, just wanted to ask first.

DropD · 2018-07-17T08:43:01Z

aiida/cmdline/commands/code.py

        }

-    def cli(self, *args):  # pylint: disable=unused-argument,no-self-use
-        verdi.main()
-
    # pylint: disable=fixme


Never use this, instead replace old TODO comment with an issue, unless it is obviously outdated of course.
For new TODO comments: commit using --no-verify.
Reason: TODO comments should only be used to indicate that action is required before merging. In which case they should cause tests to fail.

sphuber · 2018-07-23T21:16:37Z

@ltalirz I found the problem that cause the tests to fail. You accidentally removed the .name attribute call on the entry point in the CodeBuilder, so now the EntryPoint instance is set as opposed the string name. See this commit to fix it: 0753c7c

sphuber · 2018-07-23T21:18:56Z

This is the last open PR for the verdi branch. Would it be possible to get it ready for merging by tomorrow? In that case @DropD and me can rip out the old plumbing and rely solely on click. If not, maybe we could merge anyway and open an issue for the remaining sub tasks?

ltalirz · 2018-07-23T21:54:17Z

Don't wait for this one, it is not essential. I can fix it once I'm back from holidays. Cheers from Chicago!

On Mon, Jul 23, 2018, 16:18 Sebastiaan Huber ***@***.***> wrote: This is the last open PR for the verdi branch. Would it be possible to get it ready for merging by tomorrow? In that case @DropD <https://github.com/DropD> and me can rip out the old plumbing and rely solely on click. If not, maybe we could merge anyway and open an issue for the remaining sub tasks? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1737 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABASPdA6ewWvaaKIPBOTF9E3xwcuXGw0ks5uJj3BgaJpZM4VJXxW> .

-- Sent from mobile phone

The `.name` attribute call was removed, leading the EntryPoint instance to be set, instead of its `simpleplugins.templatereplacer` name

…cate

codecov-io · 2018-07-24T12:17:48Z

Codecov Report

Merging #1737 into develop will increase coverage by 0.09%.
The diff coverage is 90.09%.

@@             Coverage Diff             @@
##           develop    #1737      +/-   ##
===========================================
+ Coverage    67.28%   67.37%   +0.09%     
===========================================
  Files          320      320              
  Lines        33178    33264      +86     
===========================================
+ Hits         22325    22413      +88     
+ Misses       10853    10851       -2

Impacted Files	Coverage Δ
aiida/cmdline/params/options/__init__.py	`100% <ø> (ø)`	⬆️
aiida/cmdline/commands/cmd_user.py	`92.68% <ø> (ø)`	⬆️
aiida/control/code.py	`83.33% <86.66%> (+8.62%)`	⬆️
aiida/cmdline/commands/cmd_code.py	`92.37% <91.54%> (-0.27%)`	⬇️
aiida/orm/implementation/sqlalchemy/group.py	`88.26% <0%> (+0.51%)`	⬆️
aiida/orm/implementation/general/code.py	`79.49% <0%> (+1.25%)`	⬆️

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 a48ae14...8d080e4. Read the comment docs.

DropD

@ltalirz should rebase this in order to be attributed as author for the changes.

* also: fix info msg for `verdi code delete`

ltalirz · 2018-08-03T14:01:15Z

I've now added a test for the non-interactive version and for the interactive version.
The non-interactive test passes. For the interactive one, the test exits with exit status 2 but I don't know why (testing the interactive version by hand works) and I have no idea how to debug it.

fix interactive unit test for verdi code duplicate

ltalirz · 2018-08-06T12:52:27Z

@DropD Please have a look, from my point of view it can be merged.

Currently, code_duplicate duplicates the options of code setup (with some extra stuff to set the defaults from the code to be duplicated).
This results in quite a bit of code duplication but I don't know how to avoid it - is there another way of copying the options from verdi code setup into verdi code duplicate without actually copying the lines for the click arguments/options?

P.S. When you said I should rebase this in order to be credited, is this still needed?
I.e. when I squash and merge, will it not be my commit?

sphuber · 2018-08-14T13:38:26Z

Why can't we just define the common options/arguments once in the cmdline.params.options file and just override those keyword arguments that are different when you decorate the actual command?

As far as the rebase goes, since you have added the last few commits, I think the attribution will go to you when we squash and merge

Finally, you put in the PR header that this fixes #1788 , but that does not seem to be the case?

ltalirz · 2018-08-20T08:15:43Z

Why can't we just define the common options/arguments once in the cmdline.params.options file and just override those keyword arguments that are different when you decorate the actual command?

@sphuber I started doing this but I'm not sure this belongs together with the other options since it is rather code-specific (and this is not always obvious from the name of the option):

ON_COMPUTER = OverridableOption(
    '--on-computer/--store-in-db',
    is_eager=False,
    default=True,
    prompt='Installed on target computer?')

REMOTE_ABS_PATH = OverridableOption(
    '--remote-abs-path',
    prompt='Remote absolute path',
    type=types.AbsolutePathParamType(dir_okay=False),
    help=('[if --on-computer]: the absolute path to the executable on the remote machine'))

CODE_FOLDER = OverridableOption(
    '--code-folder',
    prompt='Local directory containing the code',
    type=click.Path(file_okay=False, exists=True, readable=True),
    help=('[if --store-in-db]: directory containing the executable and all other files necessary for running it'))

CODE_REL_PATH = OverridableOption(
    '--code-rel-path',
    prompt='Relative path of executable inside code folder',
    type=click.Path(dir_okay=False),
    help=('[if --store-in-db]: relative path of the executable ' + \
          'inside the code-folder'))

If you like, I can still define them as OverridableOptions inside cmd_code.py (or cmd_code_options.py, whatever...) and then reuse them.

Finally, you put in the PR header that this fixes #1788 , but that does not seem to be the case?

Good catch - I must have misread the PR title

sphuber · 2018-08-21T07:56:55Z

I don't think it would be too big of a problem too put them in the general options.py even though they are a bit specific, because there are already some other options that are also specific to just a few commands. However, I would also be fine with just defining them as OverridableOptions in cmd_code.py

ltalirz · 2018-08-30T09:00:15Z

@sphuber I am getting error messages (see particularly sqlalchemy build on travis) that seem to be entirely unrelated to my PR.
Already the bug fixed in my last commit is something where I don't understand how it could end up here (I didn't touch verdi user).
Do you have an idea what could be going wrong here?

sphuber · 2018-08-30T10:38:10Z

I have no clue, the build simply seems to timeout because one of the unittests seems stuck. Do the tests run locally? I have at least a problem one my local workstation that the test that tries to delete a profile aiida.backends.test.cmdline.commands.profile hangs indefinitely. I think that might have to do with sudo issues, but not sure. Not sure if that is the same one on Travis and if so why all of sudden this is happening. Unfortuately I am busy in meetings all day, so cannot look in detail now

…licate

@sphuber

according to @sphuber, rebasing should not be necessary

sphuber

Looks good, thanks @ltalirz

ltalirz added 5 commits July 9, 2018 11:42

first version of verdi code duplicate

4fe68df

Still to do: * add possibility to override parameters * store duplicated code * add tests

add functionality to update label

3f04725

interactive updating of duplicated code

bad1617

still to do * add option to hide previous code * add tests

verdi code duplicate: add --hide-original

825919b

try to fix travis bug

0a4c3dc

DropD reviewed Jul 17, 2018

View reviewed changes

Merge branch 'verdi' into add_verdi_code_duplicate

4a2cb7c

sphuber added 2 commits July 24, 2018 12:46

Dereference name attribute of plugin variable in CodeBuilder

57ec177

The `.name` attribute call was removed, leading the EntryPoint instance to be set, instead of its `simpleplugins.templatereplacer` name

Merge remote-tracking branch 'origin/verdi' into add_verdi_code_dupli…

414aa76

…cate

DropD approved these changes Jul 24, 2018

View reviewed changes

DropD previously requested changes Jul 24, 2018

View reviewed changes

sphuber changed the base branch from verdi to develop July 27, 2018 07:48

Merge branch 'develop' into add_verdi_code_duplicate

ccb3cd9

ltalirz self-assigned this Aug 3, 2018

ltalirz added 3 commits August 3, 2018 12:22

add failing test

ad7b954

fix non-interactive code duplicate

bb98f66

* also: fix info msg for `verdi code delete`

add failing interactive test

f00491b

fix interactive unit test

dbcd78d

fix interactive unit test for verdi code duplicate

Merge branch 'develop' into add_verdi_code_duplicate

38aa998

ltalirz added 3 commits August 30, 2018 09:51

reuse code-specific options in verdi code duplicate

6be4dfd

Merge branch 'develop' into add_verdi_code_duplicate

a74c605

fix bug in verdi user

24c246d

ltalirz added 4 commits September 3, 2018 12:39

fix prompting behavior

3ea33cd

Merge remote-tracking branch 'origin/develop' into add_verdi_code_dup…

8031ea3

…licate

update docs

4730883

try fixing python3.6 pre-commit tests

8d080e4

ltalirz changed the title ~~[WIP] Add verdi code duplicate~~ Add verdi code duplicate Sep 3, 2018

sphuber approved these changes Sep 3, 2018

View reviewed changes

sphuber merged commit a575e8c into aiidateam:develop Sep 3, 2018

ltalirz deleted the add_verdi_code_duplicate branch October 21, 2018 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verdi code duplicate #1737

Add verdi code duplicate #1737

ltalirz commented Jul 10, 2018 •

edited by sphuber

Loading

ltalirz commented Jul 14, 2018

DropD Jul 17, 2018 •

edited

Loading

sphuber commented Jul 23, 2018

sphuber commented Jul 23, 2018

ltalirz commented Jul 23, 2018 via email

codecov-io commented Jul 24, 2018 •

edited

Loading

DropD left a comment

ltalirz commented Aug 3, 2018 •

edited

Loading

ltalirz commented Aug 6, 2018

sphuber commented Aug 14, 2018 •

edited

Loading

ltalirz commented Aug 20, 2018 •

edited

Loading

sphuber commented Aug 21, 2018

ltalirz commented Aug 30, 2018 •

edited

Loading

sphuber commented Aug 30, 2018

sphuber left a comment

Add verdi code duplicate #1737

Add verdi code duplicate #1737

Conversation

ltalirz commented Jul 10, 2018 • edited by sphuber Loading

ltalirz commented Jul 14, 2018

DropD Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

sphuber commented Jul 23, 2018

sphuber commented Jul 23, 2018

ltalirz commented Jul 23, 2018 via email

codecov-io commented Jul 24, 2018 • edited Loading

Codecov Report

DropD left a comment

Choose a reason for hiding this comment

ltalirz commented Aug 3, 2018 • edited Loading

ltalirz commented Aug 6, 2018

sphuber commented Aug 14, 2018 • edited Loading

ltalirz commented Aug 20, 2018 • edited Loading

sphuber commented Aug 21, 2018

ltalirz commented Aug 30, 2018 • edited Loading

sphuber commented Aug 30, 2018

sphuber left a comment

Choose a reason for hiding this comment

ltalirz commented Jul 10, 2018 •

edited by sphuber

Loading

DropD Jul 17, 2018 •

edited

Loading

codecov-io commented Jul 24, 2018 •

edited

Loading

ltalirz commented Aug 3, 2018 •

edited

Loading

sphuber commented Aug 14, 2018 •

edited

Loading

ltalirz commented Aug 20, 2018 •

edited

Loading

ltalirz commented Aug 30, 2018 •

edited

Loading