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

Remove explicit PortableCode check in presubmit #5666

Merged
merged 4 commits into from
Sep 27, 2022

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Sep 26, 2022

As mentioned at #5664 (comment), in this PR I try to remove the explicit call of the PortableCode in presubmit by adding a presubmit_check helper function to code classes.

@unkcpz unkcpz requested a review from sphuber September 26, 2022 13:47
aiida/orm/nodes/data/code/abstract.py Outdated Show resolved Hide resolved
Comment on lines 103 to 109
def presubmit_check(self, folder: Folder):
"""check the validity when calling in presubmit.

:param folder: a SandboxFolder that can be used to write calculation input files and the scheduling script.
:raises PluginInternalError: The plugin created a file that is also the executable name!.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

If we make it concrete in AbstractCode this is no longer needed

Comment on lines 96 to 101
def presubmit_check(self, folder: Folder):
if str(self.filepath_executable) in folder.get_content_list():
raise exceptions.PluginInternalError(
f'The plugin created a file {self.filepath_executable} that is also the executable name!'
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would update the docstring of the method from the AbstractCode to say:

        :raises PluginInternalError: The ``CalcJob`` plugin created a file that has the same relative filepath as the
            executable for this portable code.

Co-authored-by: Sebastiaan Huber <[email protected]>
@unkcpz unkcpz force-pushed the refac/portable-remove-from-presubmit branch 2 times, most recently from 49d534f to a1ef420 Compare September 26, 2022 18:13
@unkcpz unkcpz requested a review from sphuber September 26, 2022 18:13
@sphuber
Copy link
Contributor

sphuber commented Sep 26, 2022

Cheers @unkcpz . This is good to go. If you can just fix the docs (correcting the reference to CalcJob.presubmit) this can be merged. P.S. I'm sorry, I said to change the type hint from Folder to SandboxFolder, but you are right, it should really be Folder, because for dry-runs, it can receive a SubmitTestFolder, which is another subclass of Folder.

@unkcpz unkcpz force-pushed the refac/portable-remove-from-presubmit branch from a1ef420 to 50a087a Compare September 26, 2022 22:18
@unkcpz unkcpz force-pushed the refac/portable-remove-from-presubmit branch from 50a087a to ac29e50 Compare September 26, 2022 22:29
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.

Great, thanks a lot @unkcpz

@sphuber sphuber merged commit a7775f3 into aiidateam:main Sep 27, 2022
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