-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(launcher): allow local launcher to work with xpress #2251
base: dev
Are you sure you want to change the base?
Conversation
649422c
to
4215f7e
Compare
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.
Main comments are about log management, as discussed
exc_info=e, | ||
) | ||
del self.job_id_to_study_id[str(uuid)] | ||
if process.returncode == 0: |
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.
In slurm laucher we still call the import output when it has failed, we need to understand if it makes sense
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.
From my understanding, if the simulator fails, the output is not retrieved from calin, therefore the _import_output
fails. But with the local launcher the output exists and therefore it can be retrieved. I think we should keep the code as it is, else i would have to touch code shared with the slurm part
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.
Ok. Indeed if we still wanted to import the output, we should have some kind of status to say that it's a "failed output".
I wonder if we should not also delete the import output in slurm implementation, to clarify the behaviour ?
ef11512
to
ffcecd9
Compare
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.
Looks very good!
We need to do a few things for the desktop version:
- adapt the config in resources/antares-desktop-fs
- add the new empty directory in resources/antares-desktop-fs
- we also need to write code in the installer to do the same thing to upgrade existing installations ...
4fce0ae
to
6098d99
Compare
Will fix [ANT-2472]
Introduces few changes: