-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fixing adding of inputfiles to the GaudiExec when submitting to Dirac #822
Conversation
I've also fixed |
@mesmith75 What are your thoughts on this? |
@rob-c If this works then great. The PR I made is not very good. I guess we really want this to start supporting DiracFiles in the near future but for now just LocalFiles is pretty necessary so it should go in for 6.2.1. |
@mesmith75 We already support |
I'll check the code works as expected in a few min, I'm debugging a few different things at once now. |
The code for this looks ok. I would like this for 6.2.1 as it is quite an important fix. |
I'm trying to check now and facing the usual grid problems... I'll decompose the inputsandbox manually. |
for file_ in app.getJobObject().inputfiles: | ||
if isinstance(file_, LocalFile): | ||
shutil.copy(os.path.join(file_.localDir, os.path.basename(file_.namePattern)), shared_dir) | ||
input_files.append(file._namePattern) |
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.
Should this be file_.namePattern
?
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.
yes I think it should, good catch!
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.
I also think the input_files needs to append the full path as well as the name otherwise the tarring cannot find it.
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.
it does in my testing version of the PR, uploading now
I'm looking to merge today, everything appears working from a test job. I'll add something about this to the release notes when this gets merged. |
This should supersede #815. I'm checking for unsupported filetypes (we don't correctly support moving files in a nice way and I don't want to add botch code to work around what has been fixed in 6.3)
This should add inputfiles which vary between subjobs to the inputsandbox as there is no way in principle of collecting these before the subjob is being prepared so might as well send these along with the sandbox which slows submission but this is only for very complex job configurations.
I'll try and test this now.