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

assgin clean_workdir to plugin's builder #667

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Apr 8, 2024

fix #666

If a plugin has clean_workdir default to True, even if the user didn't select the clean_workdir box, the plugin still cleans the workdir. This PR assigns the clean_workdir to the plugin's builder to make sure the clean_workdir is consistent.

# add plugin workchain
for name, entry_point in plugin_entries.items():
if name in properties:
plugin_builder = entry_point["get_builder"](
codes, structure, copy.deepcopy(parameters), **kwargs
)
# assume all builder has a input "clean_workdir"

Choose a reason for hiding this comment

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

Do we need to add something to the documentation for avoid a ext plugin to cause a fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out this. Indeed, there is a risk here if the workchain dose not have a clean_workdir input. I just modified it to check if the plugin has a clean_workdir input.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I think it should be safe, and no need to add the docs.

Copy link
Member

@AndresOrtegaGuerrero AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

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

LGMT!

@AndresOrtegaGuerrero
Copy link
Member

@superstar54 before you merge, do we have a test, to check if the builder set the clean_workdir ? in the workchains ?

@superstar54
Copy link
Member Author

do we have a test, to check if the builder set the clean_workdir ? in the workchains ?

For all the plugins we have (bands, pdos, xas, xps etc), I think the default clean_workdir is False, so we can not make a test using them. But I test this using the aiida-quantumespresso-hp, which has a clean_workdir default to be True, and it works.

@AndresOrtegaGuerrero
Copy link
Member

do we have a test, to check if the builder set the clean_workdir ? in the workchains ?

For all the plugins we have (bands, pdos, xas, xps etc), I think the default clean_workdir is False, so we can not make a test using them. But I test this using the aiida-quantumespresso-hp, which has a clean_workdir default to be True, and it works.

Is it possible to just check the generated builder , to check when set clean_workdir is true ? is not then is ok , you can merge.

@superstar54
Copy link
Member Author

Is it possible to just check the generated builder , to check when set clean_workdir is true ? is not then is ok , you can merge.

Yes, one can check the builder. But the builder test has been removed from this PR ##644. We will add a new builder test in the future.

@superstar54 superstar54 merged commit 8da3db9 into aiidalab:main Apr 8, 2024
14 checks passed
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.

clean_workdir failed for plugins
2 participants