-
Notifications
You must be signed in to change notification settings - Fork 82
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 implemention of CalcJobImporter
for pw.x
calculations
#847
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sphuber! Besides the comments in the review, there is also the following:
- We should also update the corresponding documentation. Happy to have a crack at this!
- Can we now remove/deprecate the outdated
PwimmigrantCalculation
class? Does this even still work? - After seeing the calculation job importing in action, I was wondering about the choice to base the design on a
remote_folder
input that is recognised by the engine. A side effect of this is that all calculation jobs will now have theremote_folder
input, which is only really used for the import feature and can potentially be confused with other inputs (e.g.parent_folder
). Making thecode
input optional is also potentially confusing for users, as I note in one of my comments below. I do see the argument from a provenance perspective: it makes sense that theremote_folder
is an input instead of an output for imported calculations. On the other hand, this potentially makes analysis on a combination of imported and AiiDA-run jobs more challenging since their provenance will be different. - Maybe we can add another method called
import_calculation
? Although having aparse_remote_data
method is nice, the first thing I would look for as a user is a method to directly import the folder and return the node. - It would be useful if the user could pass an (already open) transport to the method, in case they need to import a large number of calculations in a loop.
transport.copytree(remote_data.get_remote_path(), dirpath) | ||
|
||
builder = create_builder_from_file( | ||
str(dirpath), input_file_name, code, metadata or {}, pseudo_folder_path=pseudo_folder_path, **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few considerations here:
- Currently the
create_builder_from_file
doesn't seem to use thepseudo_dir
input. pseudo_folder_path
doesn't accept aPath
.- It might be possible that the pseudo folder is on a remote machine that is not a subpath of the calculation folder. I suppose we can also allow for a
RemoteData
input here, which can then use the same transport to retrieve the pseudo files.
Thanks for the review. I only opened this PR because we got a question about importing on the mailing list. I already had the branch but hadn't opened a PR yet, especially since it wasn't really done yet. That's also why I hadn't requested a review yet 😅 since I didn't want to cause too much unnecessary work. Still, it is quite useful actually that you already gave your thoughts!
Fully agree. I had a look at the docs and simply didn't know where to add it. It is such a mess. I actually bit the bullet this weekend and started reworking the docs from scratch. See this PR #848
We really should. With #848 I am anyway getting rid of the documentation.
I think the solution here is to simply improve the error message. If there is no
Fair enough, but for the alternative case of the
This is intentional. A user wanting to import a calculation is not going to call
This is a good point. Actually, we should probably go a step further and only pass a transport in. The engine can open it and call |
I suppose this one will also fix #53? |
And also #448? |
ead1dff
to
1ae4b68
Compare
The `code` input was made optional in the `CalcJob` exactly for the purpose of the importer functionality. However, the plugins themselves often actually try to access it through `self.inputs.code` since they have to set the code UUID on the `CodeInfo`.
No description provided.