Attmpt to decouple this_code setup #5664

unkcpz · 2022-09-26T11:08:13Z

Hi @sphuber, this is an attempt to move the code script generate for prepend_cmdline_params ad cmdline_params as the parameters into AbstractCode class which is required by the containerized code implementation if the code type check needs to be removed from presubmmit of calcjob.py.

I add two helper function into AbstractCode to generate prepend_cmdline_params and cmdline_params with parameters. This is the pre-requisites of the containerized code implementation.

Please have a look and let me know if it is in the right direction. Thanks!

sphuber · 2022-09-26T12:46:45Z

aiida/orm/nodes/data/code/abstract.py

@@ -11,6 +11,7 @@
 from __future__ import annotations

 import abc
+import cmd


this is not used I think

Suggested change

import cmd

sphuber · 2022-09-26T12:49:10Z

aiida/orm/nodes/data/code/installed.py

@@ -97,7 +97,7 @@ def get_executable(self) -> str:

        :return: The executable to be called in the submission script.
        """
-        return self.filepath_executable
+        return str(self.filepath_executable)


Although this change is technically correct, I wonder whether we should change the type hint instead. I think we should prefer using pathlib.Path over str wherever possible.

So I would suggest to revert this change and make get_executable return a pathlib.Path. Then you just update the callers (currently two places) to cast to str if they need to.

sphuber · 2022-09-26T12:49:18Z

aiida/orm/nodes/data/code/portable.py

@@ -90,7 +90,7 @@ def get_executable(self) -> str:

        :return: The executable to be called in the submission script.
        """
-        return self.filepath_executable
+        return str(self.filepath_executable)


See other comment

sphuber · 2022-09-26T12:50:05Z

aiida/orm/nodes/data/code/abstract.py

@@ -76,6 +77,14 @@ def get_executable(self) -> str:
        :return: The executable to be called in the submission script.
        """

+    def get_executable_cmdline_params(self, cmdline_params: list = None) -> list:


Let's try to be as specific as possible with the type hinting. Note the following notation requires to add from __future__ import annotations at the top of the file (just below the module docstring).

Suggested change

def get_executable_cmdline_params(self, cmdline_params: list = None) -> list:

def get_executable_cmdline_params(self, cmdline_params: list[str] | None = None) -> list[str]:

sphuber · 2022-09-26T12:50:53Z

aiida/orm/nodes/data/code/abstract.py

+        """Return the list of executable with its cmdline_params"""
+        return [self.get_executable()] + (cmdline_params or [])
+
+    def get_prepend_cmdline_params(self, mpi_args=None, extra_mpirun_params=None) -> list:


Suggested change

def get_prepend_cmdline_params(self, mpi_args=None, extra_mpirun_params=None) -> list:

def get_prepend_cmdline_params(self, mpi_args: list[str] | None =None, extra_mpirun_params: list[str] | None = None) -> list[str]:

unkcpz · 2022-09-26T13:14:42Z

Hi @sphuber, I accept all the suggestions, should be fine now.

unkcpz · 2022-09-26T13:19:08Z

As we discussed offline, I did not implement to also remove the explicitly calling of PortableCode from presubmit, but for sure it needs to be removed and it is very confusing at presubmit that the code object from inputs.all_nodes and this_code from calc_info appears mixed in the function. I'll try to clarify it in another PR.

for more information, see https://pre-commit.ci

unkcpz · 2022-09-26T16:58:52Z

This PR is updated, rebased and tested in #5667, sufficient for that implementation. Could you have a look @sphuber ?

sphuber

Thanks @unkcpz . If this works for the containerized PR, this is good to go. Just some suggestions for the docstrings. And I think there is still a call to Code.get_executable in the code that needs to cast to str:

aiida-core/aiida/orm/utils/builders/code.py

Line 114 in d666332

spec['remote_abs_path'] = code.get_executable()

sphuber · 2022-09-26T20:13:54Z

aiida/engine/processes/calcjobs/calcjob.py


-            cmdline_params = [str(this_code.get_executable())] + (code_info.cmdline_params or [])
+            cmdline_params = this_code.get_executable_cmdline_params(cmdline_params=code_info.cmdline_params)


Suggested change

cmdline_params = this_code.get_executable_cmdline_params(cmdline_params=code_info.cmdline_params)

cmdline_params = this_code.get_executable_cmdline_params(code_info.cmdline_params)

sphuber · 2022-09-26T20:15:42Z

aiida/orm/nodes/data/code/abstract.py

+        """Return the list of executable with its cmdline_params
+
+        :return: list of executable with its cmdline parameters."""


Suggested change

"""Return the list of executable with its cmdline_params

:return: list of executable with its cmdline parameters."""

"""Return the list of executable with its command line parameters.

:param cmdline_params: List of command line parameters provided by the ``CalcJob`` plugin.

:return: List of the executable followed by its command line parameters.

"""

sphuber · 2022-09-26T20:18:58Z

aiida/orm/nodes/data/code/abstract.py

+        """Return the list of prepend cmdline params for mpi seeting
+
+        :return: list of prepend cmdline parameters."""


Suggested change

"""Return the list of prepend cmdline params for mpi seeting

:return: list of prepend cmdline parameters."""

"""Return List of command line parameters to be prepended to the executable in submission line.

These command line parameters are typically parameters related to MPI invocations.

:param mpi_args: List of MPI parameters provided by the ``Computer.get_mpirun_command`` method.

:param extra_mpiruns_params: List of MPI parameters provided by the ``metadata.options.extra_mpirun_params`` input of the ``CalcJob``.

:return: List of command line parameters to be prepended to the executable in submission line.

"""

for more information, see https://pre-commit.ci

unkcpz · 2022-09-26T22:27:28Z

@sphuber thanks a lot! All changed.

Just some suggestions for the docstrings. And I think there is still a call to Code.get_executable in the code that needs to cast to str.

True, and also change the type hint for the legacy code class and MockCode class for the test.

sphuber

All good, if you can just update the branch (I have merged the other PR) and fix the minor comments on the docstrings

aiida/orm/nodes/data/code/abstract.py

Co-authored-by: Sebastiaan Huber <[email protected]>

for more information, see https://pre-commit.ci

unkcpz requested a review from sphuber September 26, 2022 11:08

sphuber reviewed Sep 26, 2022

View reviewed changes

unkcpz force-pushed the refact/calcjob-code-info-setup branch from a39827e to 9ac98f9 Compare September 26, 2022 13:12

unkcpz requested a review from sphuber September 26, 2022 13:14

unkcpz and others added 3 commits September 26, 2022 15:29

Attmpt to decouple this_code setup

b3ed358

[pre-commit.ci] auto fixes from pre-commit.com hooks

c9fc9cd

for more information, see https://pre-commit.ci

r-seb

3b7ada9

unkcpz force-pushed the refact/calcjob-code-info-setup branch from fd20dd9 to 3b7ada9 Compare September 26, 2022 13:29

unkcpz mentioned this pull request Sep 26, 2022

Remove explicit PortableCode check in presubmit #5666

Merged

sphuber requested changes Sep 26, 2022

View reviewed changes

unkcpz and others added 2 commits September 27, 2022 00:12

r-seb

17ac274

[pre-commit.ci] auto fixes from pre-commit.com hooks

679a46c

for more information, see https://pre-commit.ci

unkcpz requested a review from sphuber September 26, 2022 22:27

sphuber requested changes Sep 27, 2022

View reviewed changes

aiida/orm/nodes/data/code/abstract.py Outdated Show resolved Hide resolved

aiida/orm/nodes/data/code/abstract.py Outdated Show resolved Hide resolved

aiida/orm/nodes/data/code/abstract.py Outdated Show resolved Hide resolved

unkcpz and others added 3 commits September 27, 2022 11:16

Apply suggestions from code review

e2eb296

Co-authored-by: Sebastiaan Huber <[email protected]>

Merge branch 'main' into refact/calcjob-code-info-setup

7cf5991

[pre-commit.ci] auto fixes from pre-commit.com hooks

23a500c

for more information, see https://pre-commit.ci

unkcpz requested a review from sphuber September 27, 2022 09:22

f-j

c1572ec

sphuber approved these changes Sep 27, 2022

View reviewed changes

sphuber merged commit bbc3d15 into aiidateam:main Sep 27, 2022

chrisjsewell mentioned this pull request Nov 30, 2022

Add support for pathlib.Path in the Transport interface #5797

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attmpt to decouple this_code setup #5664

Attmpt to decouple this_code setup #5664

unkcpz commented Sep 26, 2022 •

edited

Loading

sphuber Sep 26, 2022

sphuber Sep 26, 2022

sphuber Sep 26, 2022

sphuber Sep 26, 2022

sphuber Sep 26, 2022

unkcpz commented Sep 26, 2022

unkcpz commented Sep 26, 2022

unkcpz commented Sep 26, 2022 •

edited

Loading

sphuber left a comment

sphuber Sep 26, 2022

sphuber Sep 26, 2022

sphuber Sep 26, 2022

unkcpz commented Sep 26, 2022

sphuber left a comment

	def get_executable_cmdline_params(self, cmdline_params: list = None) -> list:
	def get_executable_cmdline_params(self, cmdline_params: list[str] \| None = None) -> list[str]:

	def get_prepend_cmdline_params(self, mpi_args=None, extra_mpirun_params=None) -> list:
	def get_prepend_cmdline_params(self, mpi_args: list[str] \| None =None, extra_mpirun_params: list[str] \| None = None) -> list[str]:


		cmdline_params = [str(this_code.get_executable())] + (code_info.cmdline_params or [])
		cmdline_params = this_code.get_executable_cmdline_params(cmdline_params=code_info.cmdline_params)

		"""Return the list of executable with its cmdline_params

		:return: list of executable with its cmdline parameters."""

-        """Return the list of executable with its cmdline_params
-        :return: list of executable with its cmdline parameters."""
+        """Return the list of executable with its command line parameters.
+        :param cmdline_params: List of command line parameters provided by the ``CalcJob`` plugin.
+        :return: List of the executable followed by its command line parameters.
+        """

		"""Return the list of prepend cmdline params for mpi seeting

		:return: list of prepend cmdline parameters."""

-        """Return the list of prepend cmdline params for mpi seeting
-        :return: list of prepend cmdline parameters."""
+        """Return List of command line parameters to be prepended to the executable in submission line.
+        These command line parameters are typically parameters related to MPI invocations.
+        :param mpi_args: List of MPI parameters provided by the ``Computer.get_mpirun_command`` method.
+        :param extra_mpiruns_params: List of MPI parameters provided by the ``metadata.options.extra_mpirun_params`` input of the ``CalcJob``.
+        :return: List of command line parameters to be prepended to the executable in submission line.
+        """

Attmpt to decouple this_code setup #5664

Attmpt to decouple this_code setup #5664

Conversation

unkcpz commented Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Sep 26, 2022

unkcpz commented Sep 26, 2022

unkcpz commented Sep 26, 2022 • edited Loading

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Sep 26, 2022

sphuber left a comment

Choose a reason for hiding this comment

unkcpz commented Sep 26, 2022 •

edited

Loading

unkcpz commented Sep 26, 2022 •

edited

Loading