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

Change name of the Singularity plugin to Apptainer #2293

Closed
egede opened this issue Feb 22, 2024 · 13 comments · Fixed by #2303
Closed

Change name of the Singularity plugin to Apptainer #2293

egede opened this issue Feb 22, 2024 · 13 comments · Fixed by #2303

Comments

@egede
Copy link
Member

egede commented Feb 22, 2024

For the container plugins, the name of the plugin itself should be changed to Apptainer and the default binary used should be changed to apptainer. The old Singularity plugin can be kept but a deprecation warning should be issued.

@egede
Copy link
Member Author

egede commented Feb 22, 2024

@dg1223
Copy link
Contributor

dg1223 commented Feb 24, 2024

I am an open source first-timer. I would like to solve this issue if it's okay.

Just by skimming over the code and the repo, I guess the following changes need to be made:

  1. Create a copy of Singularity.py, change the script's name to Apptainer.py, the class name and corresponding dependencies
  2. Add a deprecation warning to Singularity.py
  3. Add new import statement to init.py
  4. Add a checkApptainer function to Virtualization.py. It's probably going to be an exact copy of the checkSingularity function except for its name and the "singularity" argument in the try block.
  5. Create a copy of TestUtilitiesVirtualizationCheckSingularity.py, change the script's name to TestUtilitiesVirtualizationCheckApptainer.py, the class name and corresponding dependencies.
  6. Add a deprecation warning toTestUtilitiesVirtualizationCheckSingularity.py
  7. The default binary also get updated accordingly (to apptainer). I believe some scripts in GangaCore such as TestContainerHandler.py need to be updated.

I might be missing a few other dependencies but those should come to the surface as soon as I start testing. Any feedback is appreciated.

@egede
Copy link
Member Author

egede commented Feb 26, 2024

This sounds overall good. Usually I would recommend against duplicating code and instead make a parent class that both inherit from but in this case I think duplication is actually better - it will make it easier to delete the Singularity class in a years time or so.

@dg1223
Copy link
Contributor

dg1223 commented Feb 26, 2024

Thank you. I'll start working on it then.

@egede
Copy link
Member Author

egede commented Feb 27, 2024

I do not think it is a good idea to include the use of the deprecated package without the development of a uniform system for it first.

@dg1223
Copy link
Contributor

dg1223 commented Feb 27, 2024

Sounds good!

@dg1223
Copy link
Contributor

dg1223 commented Feb 28, 2024

@egede For the PR, do you want me to squash all commits into one or keep them as they are in the PR?

@egede
Copy link
Member Author

egede commented Feb 28, 2024

We are usually not fussed about this. So you can keep them as they are.

@dg1223
Copy link
Contributor

dg1223 commented Feb 28, 2024

Do you want me to update the documentation too?

@dg1223
Copy link
Contributor

dg1223 commented Feb 28, 2024

Actually, updating the documentation can be done as a separate issue. Doesn't make sense to update it right now before the code is QA'd.

@egede
Copy link
Member Author

egede commented Feb 28, 2024

I actually think it would be good to update the documentation in the same one. The documentation is built from the code on the develop branch (that is our main branch). So the documentation will then be updated at the same time as the code is updated.

@dg1223
Copy link
Contributor

dg1223 commented Feb 28, 2024

I am keeping the Singularity documentation as it is and adding Apptainer documentation side by side (or below) as a 1-to-1 mapping since we are still going to keep Singularity alive for some time. Does that sound good?

@dg1223
Copy link
Contributor

dg1223 commented Feb 28, 2024

Opened a PR #2303

@egede egede linked a pull request Mar 1, 2024 that will close this issue
mesmith75 added a commit that referenced this issue Mar 19, 2024
#2293: Change name of the Singularity plugin to Apptainer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants