-
Notifications
You must be signed in to change notification settings - Fork 12
AEP: Add native support for containerized codes #28
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks @unkcpz for the AEP! Here a few comments from me:
It would be good to suggest already which technology we want to support (e.g. Docker, singularity and sarus?) and have working examples of how the submission strings would look like (something that actually works in one of the supercomputers we use, similar to what you already provided for Sarus - ensuring that you tested it and it works, and mentioning what command line parameters are essential to specify, what are optional. Regarding your comment on the One final thing to mention is that, in order for this to work, probably one has to go first in the supercomputer and Also, a few minor details on the text:
|
I was going to mention this as well. I think them not being split causes a lot of problems in the interface and implementation. The only question is if we can make this change with no or very little code breaking, which I don't think will be trivial. But I would definitely advocate for the container code to be a separate class and have
I haven't read the advantage of not hard-coding a computer, but if there is a real advantage, it is already possible to pass a computer at runtime through the inputs as
What container technology is supported definitely seems something that should be just part of the |
|
When you say "this information might change in the future" you mean that a computer might stop supporting, say, |
Exactly. The way you phrases it, in order to use a container plugin with a certain
I might misunderstand, but not really. The scheduler type is specified on the |
|
Sorry - I meant you "In this case, I would just define a new computer (in the same way we would define a new computer if it passes from PBS to slurm), no?" |
Sure, the question is whether in this case it is really necessary or just an arbitrary limitation of our current setup. If there really is no reason to not allow it to change on an existing computer, it would be annoying if you would be forced to nonetheless. Again, this is barring that changing it would actually affect the provenance in anyway. I agree that the simplest is to not change anything and simply require to create a new computer. |
|
Thanks @giovannipizzi Thanks @sphuber !! Sorry for the delay in the update and reply, before the meeting with CSCS yesterday, I have no strong opinions for most of the questions you mentioned @giovannipizzi ;) Now I still hold my opinions for most of the questions, I update the AEP and list them as open questions. Let's settle them down during the coding week! Just to try to clarify something as far as I know.
Eiger and daint now support both sarus and singularity.
I think for the
|
c9a77f2 to
d4c5b97
Compare
|
@unkcpz does this PR reflect the implementation route that was chosen in the end? Should this PR be closed or merged (and if it should be merged, should it be updated)? |
|
This PR should be updated and merged. I'll update it with the final decision we made and the things we implemented. |
|
Note that there is this PR on |
submittedREADME.md