ganga-devs / ganga

Ganga is an easy-to-use frontend for job definition and management
GNU General Public License v3.0
100 stars 159 forks source link

Change name of the Singularity plugin to Apptainer #2293

Closed egede closed 8 months ago

egede commented 9 months ago

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 commented 9 months ago

The code resides at https://github.com/ganga-devs/ganga/tree/develop/ganga/GangaCore/Lib/Virtualization and some corresponding tests in https://github.com/ganga-devs/ganga/tree/develop/ganga/GangaCore/test/Unit/Utility.

dg1223 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

egede commented 8 months ago

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 commented 8 months ago

Sounds good!

dg1223 commented 8 months ago

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

egede commented 8 months ago

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

dg1223 commented 8 months ago

Do you want me to update the documentation too?

dg1223 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Opened a PR #2303