Added a remotecat command for printing remote file of running calcjobs
#4861
Conversation
|
Hey @zhubonan , thanks for the contribution! We are currently doing/discussing some changes in how we are handling the connections for external resources (in particular, ssh connections), so we need to consider how this feature fits into all that. Given that it is a command line feature, it might still be useful to have something like it but perhaps the underlying workings might need to be different. I'm pinging @sphuber and @giovannipizzi if they can take a look. Anyways, I'll try to bring the subject up on the next AiiDA meeting (it is this wednesday at 15:00 CEST, these are open for participation so let me know if you want to be present). |
|
I see. This is just a small feature that I think might be useful. It does not touch the ssh interface though (only Feel free to come back to this later. |
|
+1 I'd definitely use this feature. I'm wondering if it can also be extended to monitor the calculation. I typically do this with the It would be nice to just be able to do e.g.: |
|
Hi @zhubonan , thanks for this contribution!
You could add a test to the "system tests", some of which run calculations on a slurm docker container on github actions. aiida-core/.github/system_tests/pytest/test_memory_leaks.py Lines 48 to 62 in fa8b243 I might not be the right person to review this PR but we'll discuss it at the meeting |
Right, so this is one of the things. The |
|
I think that in principle it would be ok to add this command even though we are not yet sure of how the transports may change, since if and when we change it, it should not affect the interface and so the user shouldn't be affected. That being said: in a sense this command already exists in |
@mbercx I have coded this up as well actually based on |
Codecov ReportBase: 79.69% // Head: 80.10% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4861 +/- ##
==========================================
+ Coverage 79.69% 80.10% +0.42%
==========================================
Files 545 515 -30
Lines 39150 36742 -2408
==========================================
- Hits 31195 29427 -1768
+ Misses 7955 7315 -640
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
zhubonan
force-pushed
the
pr-add-remotecat-command
branch
from
559c147
to
9770b11
Compare
|
Hey thanks @zhubonan, I'm seeing a lot of lines not covered by tests 😬 would it be possible to look into increasing the test coverage for these scenarios?
|
|
@chrisjsewell I tried to improve the coverages but it turns out additional changes are needed to support the |
sphuber
force-pushed
the
pr-add-remotecat-command
branch
2 times, most recently
from
412964f
to
0132a8b
Compare
|
@zhubonan I have rebased the PR to the latest In looking into the code more closely, I have just a question of (ab)using the |
|
@sphuber Thanks for looking into it. If there is a Perhaps we can just remove the |
Works for me. Let me create a backup branch for you on your fork and then I will remove the option and we can merge this. |
sphuber
force-pushed
the
pr-add-remotecat-command
branch
2 times, most recently
from
e6e3f76
to
294ed36
Compare
This can be usedful for monitoring the calcjobs while they are running.
Also adds a test for the `--monitor` option.
sphuber
force-pushed
the
pr-add-remotecat-command
branch
from
294ed36
to
6bcc260
Compare
sphuber
force-pushed
the
pr-add-remotecat-command
branch
from
6bcc260
to
46bb011
Compare
|
@zhubonan I cannot seem to push the branch with the monitor option code to your remote. But I will add the patch here for posterity: From 02d9fa15a508cc15513f64387adc7171e16d77cb Mon Sep 17 00:00:00 2001
From: Bonan Zhu <[email protected]>
Date: Sat, 17 Apr 2021 15:57:27 +0100
Subject: [PATCH] Added a remotecat command for printing remote file
This can be usedful for monitoring the calcjobs while they are
running.
Various minor improvements
Also adds a test for the `--monitor` option.
---
aiida/cmdline/commands/cmd_calcjob.py | 20 +++++++++++++++++++-
aiida/transports/plugins/local.py | 4 ++--
aiida/transports/plugins/ssh.py | 4 ++--
aiida/transports/transport.py | 12 ++++++++----
tests/cmdline/commands/test_calcjob.py | 11 ++++++++++-
tests/transports/test_local.py | 9 +++++++++
tests/transports/test_ssh.py | 9 +++++++++
7 files changed, 59 insertions(+), 10 deletions(-)
diff --git a/aiida/cmdline/commands/cmd_calcjob.py b/aiida/cmdline/commands/cmd_calcjob.py
index 72cec957f..da542942f 100644
--- a/aiida/cmdline/commands/cmd_calcjob.py
+++ b/aiida/cmdline/commands/cmd_calcjob.py
@@ -124,8 +124,9 @@ def calcjob_inputcat(calcjob, path):
@verdi_calcjob.command('remotecat')
@arguments.CALCULATION('calcjob', type=CalculationParamType(sub_classes=('aiida.node:process.calculation.calcjob',)))
@click.argument('path', type=str, required=False)
+@click.option('--monitor', is_flag=True, default=False, help='Monitor the file using `tail -f` instead.')
@decorators.with_dbenv()
-def calcjob_remotecat(calcjob, path):
+def calcjob_remotecat(calcjob, path, monitor):
"""Show the contents of a file in the remote working directory.
The file to show can be specified using the PATH argument. If PATH is not specified, the default output file path
@@ -135,8 +136,25 @@ def calcjob_remotecat(calcjob, path):
import sys
import tempfile
+ from aiida.common.exceptions import NotExistent
+
remote_folder, path = get_remote_and_path(calcjob, path)
+ if monitor:
+ try:
+ transport = calcjob.get_transport()
+ except NotExistent as exception:
+ echo.echo_critical(str(exception))
+
+ remote_workdir = calcjob.get_remote_workdir()
+
+ if not remote_workdir:
+ echo.echo_critical('no remote work directory for this calcjob, maybe it has not yet started running?')
+ cmds = f"-c 'tail -f {path}'"
+ command = transport.gotocomputer_command(remote_workdir, cmds)
+ os.system(command)
+ return
+
with tempfile.NamedTemporaryFile() as tmp_path:
try:
remote_folder.getfile(path, tmp_path.name)
diff --git a/aiida/transports/plugins/local.py b/aiida/transports/plugins/local.py
index c41f4f276..cc56a76a2 100644
--- a/aiida/transports/plugins/local.py
+++ b/aiida/transports/plugins/local.py
@@ -817,7 +817,7 @@ class LocalTransport(Transport):
return retval, output_text, stderr_text
- def gotocomputer_command(self, remotedir):
+ def gotocomputer_command(self, remotedir, extra_args=None):
"""
Return a string to be run using os.system in order to connect
via the transport to the remote directory.
@@ -829,7 +829,7 @@ class LocalTransport(Transport):
:param str remotedir: the full path of the remote directory
"""
- connect_string = self._gotocomputer_string(remotedir)
+ connect_string = self._gotocomputer_string(remotedir, extra_args)
cmd = f'bash -c {connect_string}'
return cmd
diff --git a/aiida/transports/plugins/ssh.py b/aiida/transports/plugins/ssh.py
index f47c9ef19..4849ce766 100644
--- a/aiida/transports/plugins/ssh.py
+++ b/aiida/transports/plugins/ssh.py
@@ -1482,7 +1482,7 @@ class SshTransport(Transport): # pylint: disable=too-many-public-methods
return (retval, b''.join(stdout_bytes), b''.join(stderr_bytes))
- def gotocomputer_command(self, remotedir):
+ def gotocomputer_command(self, remotedir, extra_args=None):
"""
Specific gotocomputer string to connect to a given remote computer via
ssh and directly go to the calculation folder.
@@ -1505,7 +1505,7 @@ class SshTransport(Transport): # pylint: disable=too-many-public-methods
further_params_str = ' '.join(further_params)
- connect_string = self._gotocomputer_string(remotedir)
+ connect_string = self._gotocomputer_string(remotedir, extra_args)
cmd = f'ssh -t {self._machine} {further_params_str} {connect_string}'
return cmd
diff --git a/aiida/transports/transport.py b/aiida/transports/transport.py
index 1f3a01c75..0031e5770 100644
--- a/aiida/transports/transport.py
+++ b/aiida/transports/transport.py
@@ -688,7 +688,7 @@ class Transport(abc.ABC):
"""
@abc.abstractmethod
- def gotocomputer_command(self, remotedir):
+ def gotocomputer_command(self, remotedir, extra_args=None):
"""
Return a string to be run using os.system in order to connect
via the transport to the remote directory.
@@ -700,6 +700,7 @@ class Transport(abc.ABC):
* A reasonable error message is produced if the folder does not exist
:param str remotedir: the full path of the remote directory
+ :param str extra_args: optional extra arguments to be passed to the shell
"""
@abc.abstractmethod
@@ -815,13 +816,16 @@ class Transport(abc.ABC):
def has_magic(self, string):
return self._MAGIC_CHECK.search(string) is not None
- def _gotocomputer_string(self, remotedir):
+ def _gotocomputer_string(self, remotedir, extra_args=None):
"""command executed when goto computer."""
connect_string = (
""" "if [ -d {escaped_remotedir} ] ;"""
- """ then cd {escaped_remotedir} ; {bash_command} ; else echo ' ** The directory' ; """
+ """ then cd {escaped_remotedir} ; {bash_command} {extra_args}; else echo ' ** The directory' ; """
"""echo ' ** {remotedir}' ; echo ' ** seems to have been deleted, I logout...' ; fi" """.format(
- bash_command=self._bash_command_str, escaped_remotedir="'{}'".format(remotedir), remotedir=remotedir
+ bash_command=self._bash_command_str,
+ extra_args=extra_args or '',
+ escaped_remotedir="'{}'".format(remotedir),
+ remotedir=remotedir
)
)
diff --git a/tests/cmdline/commands/test_calcjob.py b/tests/cmdline/commands/test_calcjob.py
index a763b1275..801761bf6 100644
--- a/tests/cmdline/commands/test_calcjob.py
+++ b/tests/cmdline/commands/test_calcjob.py
@@ -334,9 +334,11 @@ class TestVerdiCalculation:
assert len(get_result_lines(result)) == 1
assert get_result_lines(result)[0] == '5'
- def test_calcjob_remotecat(self):
+ def test_calcjob_remotecat(self, monkeypatch):
"""Test the remotecat command that prints the remote file for a given calcjob"""
# Specifying no filtering options and no explicit calcjobs should exit with non-zero status
+ import os
+
options = []
result = self.cli_runner.invoke(command.calcjob_remotecat, options)
assert result.exception is not None, result.output
@@ -357,3 +359,10 @@ class TestVerdiCalculation:
options = [str(self.result_job.uuid), 'fileA.txt']
result = self.cli_runner.invoke(command.calcjob_remotecat, options)
assert result.stdout == 'test stringA'
+
+ # To test the ``--monitor`` option, mock the ``os.system`` command and simply print the command it receives.
+ with monkeypatch.context() as ctx:
+ ctx.setattr(os, 'system', lambda x: print(x)) # pylint: disable=unnecessary-lambda
+ options = ['--monitor', str(self.result_job.uuid), 'fileA.txt']
+ result = self.cli_runner.invoke(command.calcjob_remotecat, options)
+ assert "bash -l -c 'tail -f fileA.txt'" in result.stdout
diff --git a/tests/transports/test_local.py b/tests/transports/test_local.py
index fa88e0046..102f0323d 100644
--- a/tests/transports/test_local.py
+++ b/tests/transports/test_local.py
@@ -61,3 +61,12 @@ def test_gotocomputer():
"""echo ' ** /remote_dir/' ; echo ' ** seems to have been deleted, I logout...' ; fi" """
)
assert cmd_str == expected_str
+
+ cmd_str = transport.gotocomputer_command('/remote_dir/', "-c 'echo Hello World'")
+
+ expected_str = (
+ """bash -c "if [ -d '/remote_dir/' ] ;"""
+ """ then cd '/remote_dir/' ; bash -l -c 'echo Hello World'; else echo ' ** The directory' ; """
+ """echo ' ** /remote_dir/' ; echo ' ** seems to have been deleted, I logout...' ; fi" """
+ )
+ assert cmd_str == expected_str
diff --git a/tests/transports/test_ssh.py b/tests/transports/test_ssh.py
index 8b2043f87..958748723 100644
--- a/tests/transports/test_ssh.py
+++ b/tests/transports/test_ssh.py
@@ -124,6 +124,15 @@ def test_gotocomputer():
)
assert cmd_str == expected_str
+ cmd_str = transport.gotocomputer_command('/remote_dir/', '-c "echo Hello World"')
+
+ expected_str = (
+ """ssh -t localhost -o ProxyCommand='ssh -W localhost:22 localhost' "if [ -d '/remote_dir/' ] ;"""
+ """ then cd '/remote_dir/' ; bash -c "echo Hello World"; else echo ' ** The directory' ; """
+ """echo ' ** /remote_dir/' ; echo ' ** seems to have been deleted, I logout...' ; fi" """
+ )
+ assert cmd_str == expected_str
+
def test_gotocomputer_proxyjump():
"""Test gotocomputer"""
--
2.25.1 |
|
Thanks for the patch 😄 . I think I will just delete my branch since the PR is now merged. |
This is a command that I found useful - perhaps it is good to commit it to the core.
It can be useful for monitoring the calcjob while they are running.
I have not yet able to add tests for it - the existing test framework for
calcjobcommands does not include remote contents (the nodes are extracted from an archive). Not sure how to make the test work, perhaps the only way is to mock thegetfilemethod of theRemoteDatanode?