Skip to content

Implement verdi computer duplicate #1937

Merged
sphuber merged 3 commits into
aiidateam:developfrom
sphuber:fix_1788_verdi_computer_duplicate
Sep 5, 2018
Merged

Implement verdi computer duplicate #1937
sphuber merged 3 commits into
aiidateam:developfrom
sphuber:fix_1788_verdi_computer_duplicate

Conversation

@sphuber
Copy link
Copy Markdown
Contributor

@sphuber sphuber commented Sep 4, 2018

Fixes #1788

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 4, 2018

Codecov Report

Merging #1937 into develop will increase coverage by 0.05%.
The diff coverage is 87.2%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1937      +/-   ##
===========================================
+ Coverage    67.47%   67.53%   +0.05%     
===========================================
  Files          321      321              
  Lines        33195    33275      +80     
===========================================
+ Hits         22399    22472      +73     
- Misses       10796    10803       +7
Impacted Files Coverage Δ
aiida/control/code.py 83.33% <ø> (ø) ⬆️
aiida/cmdline/commands/cmd_code.py 92.37% <ø> (ø) ⬆️
aiida/control/computer.py 83.72% <100%> (+2.63%) ⬆️
aiida/cmdline/params/types/nonemptystring.py 81.81% <100%> (ø) ⬆️
aiida/cmdline/commands/cmd_computer.py 80.19% <84.61%> (+0.94%) ⬆️
aiida/orm/implementation/django/computer.py 79.47% <0%> (+0.52%) ⬆️
aiida/orm/implementation/sqlalchemy/computer.py 81.72% <0%> (+0.53%) ⬆️

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 5e9b372...5d56dcc. Read the comment docs.

Copy link
Copy Markdown
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber
I'm requesting some changes here but I'm OK to approve if you've looked at them and decide against.

Comment thread aiida/cmdline/commands/cmd_computer.py Outdated
echo.echo_info('pk: {}, uuid: {}'.format(computer.pk, computer.uuid))

echo.echo_info("Note: before using it with AiiDA, configure it using the command")
echo.echo_info(" verdi computer configure {} {}".format(computer.get_transport_type(), computer.name))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In interactive mode, we should simply prompt the user should for this (because it almost always needs to happen).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Meaning: Configure computer ... now? [Y/n] )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you now how this would work? I know I can do something like ctx.invoke(click_command, **kwargs) to invoke the command, but if I don't pass anything in kwargs, it complains that Computer is None in the configure command. If I do pass it, I get the exception TypeError: transport_configure_command() got multiple values for keyword argument 'computer'. Is this complicated because the configure commands are created dynamically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment thread aiida/cmdline/commands/cmd_computer.py Outdated
echo.echo_info("Note: before using it with AiiDA, configure it using the command")
echo.echo_info(" verdi computer configure {} {}".format(computer.get_transport_type(), computer.name))
echo.echo_info("(Note: machine_dependent transport parameters cannot be set via ")
echo.echo_info("the command-line interface at the moment)")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to give a bit more details here/be more constructive?
E.g. "Note: For setting[property] use [function] from the python interface"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that the description is vague, I still don't really understand what is meant here. Does it relate to minimum transport open interval and stuff like that? If so, in any case I don't think that listing them here is going to make it more clear. Probably we should in that case just reference to a place in the documentation, until they get implemented in the CLI. Should this go in this PR, or should we open separate issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK after discussion, removed these lines as they will be implemented in the future and the current information was just plain confusing

* **test**: tests if the current user (or a given user) can connect to the computer and if basic operations perform as expected (file copy, getting the list of jobs in the scheduler queue, ...)
* **update**: change configuration of a computer. Works only if the computer node is a disconnected node in the database (has not been used yet)
* **duplicate**: setup a new computer, starting from the settings of an existing one
* **update**: [deprecated: use ``verdi computer duplicate`` instead] change configuration of a computer. Works only if the computer node is a disconnected node in the database (has not been used yet)
Copy link
Copy Markdown
Member

@ltalirz ltalirz Sep 4, 2018

Choose a reason for hiding this comment

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

Not even sure we should include deprecated functions in the user documentation.
Once the deprecation is clear from the verdi output, I think we don't need to mention it here.

(btw: in the case of verdi code, update is not just deprecated - it also does not work)

Copy link
Copy Markdown
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Just tested the automatic configure locally and it's fantastic!
One of these things where people will wonder why it hasn't been like this all along.

except ValidationError as err:
echo.echo_critical('unable to store the computer: {}. Exiting...'.format(err))
else:
echo.echo_success('stored computer {}<{}>'.format(computer.name, computer.pk))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

never saw this pattern before - learned something new today ;-)

@sphuber sphuber merged commit a6ca7b2 into aiidateam:develop Sep 5, 2018
@sphuber sphuber deleted the fix_1788_verdi_computer_duplicate branch September 5, 2018 06:50
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 this pull request may close these issues.

3 participants