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

Fixed Issue #552 #553

Merged
merged 10 commits into from
Dec 19, 2023
Merged

Fixed Issue #552 #553

merged 10 commits into from
Dec 19, 2023

Conversation

andres-fr
Copy link
Contributor

This PR implements the fix proposed in the issue:

  1. Created a small script to convert Dockerfile into Singularity.def in a way that avoids the %files command.
  2. Used said script to generate Singularity.def (tested on the cluster, works)
  3. Updated README docs.

Note that the PR also includes the generated Singularity.def file. This refactoring IMO has several advantages for the end user:

  • They don't need to install spython, one dependency less
  • Less steps: also no need to generate the recipe
  • The recipe itself is known to work (at least in our cluster)
  • If users wish to customize their image creation, they can use the newly provided script as a starting point, much more flexible than the previous command.

Let me know if you have any comments!
Cheers
Andres

…def and its creation process. Updated respective section in README
@andres-fr andres-fr requested a review from a team as a code owner October 26, 2023 02:59
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@andres-fr
Copy link
Contributor Author

recheck

1 similar comment
@andres-fr
Copy link
Contributor Author

recheck

@andres-fr
Copy link
Contributor Author

I'm not sure I'm doing the CLA right... sorry for the noise, I'll check again tomorrow.

runame
runame previously approved these changes Oct 30, 2023
Copy link
Contributor

@runame runame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this should fix the issue. We can just address any remaining problems with follow-up PRs.

docker/scripts/singularity_converter.py Show resolved Hide resolved
@andres-fr
Copy link
Contributor Author

Hi @runame thanks for checking it out.
I've just submitted the signed CLA form to the admins, and pushed the quotation mark fix. Sorry about that one.
Let me know if there is anything else needed from my side to get this PR through.

@andres-fr
Copy link
Contributor Author

Hi! what is the status on this?
Now that I pass the CLA bot check, should we get this through before it stalls too much?

I can update/resolve the README and figure out the isort issue, that seems all is left

@andres-fr
Copy link
Contributor Author

Alright, now isort should not complain, and resolved the README conflict.
Let me know if any further actions are needed!

@priyakasimbeg priyakasimbeg changed the base branch from main to dev November 13, 2023 20:47
Copy link
Contributor

@priyakasimbeg priyakasimbeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Andres, thank you for taking ownership of this issue and helping to get our singularity workflow in better shape!
I just caught one small typo but other than that LGTM.
Also, it seems like the yapf test is failing. You can fix the formatting in the converter script by running yapf -i -r -vv -p docker/scripts/singularity_converter.py

docker/scripts/singularity_converter.py Outdated Show resolved Hide resolved
@runame runame requested a review from priyakasimbeg November 14, 2023 19:18
priyakasimbeg
priyakasimbeg previously approved these changes Dec 12, 2023
@priyakasimbeg
Copy link
Contributor

Hi Andres, there is a merge conflict left on this PR after we reorganized the documentation. Can you move your documentation updates to the GETTING_STARTED.md under the section Using Singularity Instead of Docker please? link to section

@andres-fr
Copy link
Contributor Author

Dear @priyakasimbeg, thanks for taking another look into this. I see that you've moved some of the README contents into GETTING_STARTED. This seemed to include 100% of the merge conflict, so I've:

  1. Deleted all merge conflict contents from the README, leaving it exactly as it is on your current main branch. No conflicts should remain.
  2. Updated the singularity section from GETTING_STARTED with the contents of this PR, plus some minor phrasing updates.

Let me know if this looks OK, and thanks again!

Copy link
Contributor

@priyakasimbeg priyakasimbeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for resolving the merge conflicts! I just caught one unintended change in the README.md. I don't think we want to remove the TLDR pytorch installation instructions. Could you add it back please?

README.md Outdated Show resolved Hide resolved
@priyakasimbeg priyakasimbeg merged commit bfa2ba2 into mlcommons:dev Dec 19, 2023
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants