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 ability for verdi process report to automatically query last node #6171

Closed
wants to merge 17 commits into from

Conversation

ljbeal
Copy link
Contributor

@ljbeal ljbeal commented Nov 6, 2023

Adds the -e and --end flags to verdi process report, replacing the requirement of specifiying a node.

Instead, report will query verdi process list -a and call on the last pk found. This makes the process reproducible in a single line, and removes a step in debugging a calculation.

This feature also attempts to collect the remote dir of the node, for further debugging.

It's highly probable that there's a cleaner way of implementing this, but it has been helpful for my workflows, feel free to advise (or go ahead any make) any changes to the implementation!

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @ljbeal . I think this could indeed be useful. I have added some suggestions for the implementation. I think it would also make sense to add this to verdi process show and verdi process status. If you agree, we can move the option definition to aiida.cmdline.params.options.main as a reusable option. We would also have to add some tests. I can give pointers if you'd like, or take over if you don't have the time

aiida/cmdline/commands/cmd_process.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_process.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_process.py Outdated Show resolved Hide resolved
Comment on lines 210 to 214
try:
remote_dir = node.outputs.remote_folder.get_attribute('remote_path')
echo.echo(f'Remote Directory: {remote_dir}')
except AttributeError:
echo.echo(f'No Remote Directory Found')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's purely for debugging reasons, before I learned about verdi calcjob gotocomputer. This can either be removed or set to echo_info.

I still use it occasionally since it lets me cat {remote_dir}/_aiidasubmit.sh without leaving the current working dir, for example. But just like the Reporting for final node above, it's not strictly required info.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's purely for debugging reasons, before I learned about verdi calcjob gotocomputer. This can either be removed or set to echo_info.

I don't like it so much because it is specific to CalcJobs and doesn't apply to any other processes like calcfunctions, WorkChains etc.

I still use it occasionally since it lets me cat {remote_dir}/_aiidasubmit.sh without leaving the current working dir, for example.

This will only work if your CalcJob ran on the localhost though. If you ran it on a remote computer, this won't work and you would anyway have to use verdi calcjob gotocomputer.

But just like the Reporting for final node above, it's not strictly required info.

The difference is that this applies to all processes. In addition, it can actually help to prevent confusion. Since when you use this flag you think you might get the last node, but another one could have run in the meantime (if the daemon is running and has other proceses running), and so really you got another node than you were anticipating. In this case an explicit reference of exactly which node was matched can prevent confusion.

@ljbeal
Copy link
Contributor Author

ljbeal commented Nov 10, 2023

Hi @sphuber, thanks for the response, and apologies for the delay in my own. The suggestions you made are good ones, I have responded in place.

I like the idea of making it a common variable, I found myself wanting to implement this for verdi node show at the very least.

Indeed I was lazy with the tests, I'll get on that as a priority once I get aiida-bigdft replaced on PyPi.

As for the pointers vs you doing the work, it's entirely up to you. I'm happy to do it myself, though if you choose to go ahead and make the necessary changes I'll definitely go take a look at the git diff. I get the sense that it will be useful info on the plugin side of things in either case.

@sphuber
Copy link
Contributor

sphuber commented Nov 10, 2023

As for the pointers vs you doing the work, it's entirely up to you. I'm happy to do it myself, though if you choose to go ahead and make the necessary changes I'll definitely go take a look at the git diff. I get the sense that it will be useful info on the plugin side of things in either case.

For me there is no rush and I am happy to wait. It is just that I think this is a useful feature and so think it would be good to get it in, but if requests for adding tests block that because you don't have the time for it, I will happily pitch in.

@sphuber
Copy link
Contributor

sphuber commented Feb 9, 2024

Superseded by #6283

@sphuber sphuber closed this Feb 9, 2024
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.

2 participants