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

ARM container numcodecs fix #174

Merged

Conversation

ktangsali
Copy link
Collaborator

@ktangsali ktangsali commented Sep 28, 2023

Modulus Pull Request

Description

Workaround fix for #175

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.

Dependencies

@ktangsali ktangsali marked this pull request as draft September 28, 2023 00:40
@ktangsali ktangsali self-assigned this Sep 28, 2023
@ktangsali ktangsali marked this pull request as ready for review September 28, 2023 16:13
@ktangsali
Copy link
Collaborator Author

/blossom-ci

Copy link
Collaborator

@akshaysubr akshaysubr left a comment

Choose a reason for hiding this comment

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

This PR looks good overall. But maybe we should think about how we can make Dockerfile changes like this and test container builds it in an environment that mimics an end user's and doesn't have all these custom wheels available.

Dockerfile Show resolved Hide resolved
@ktangsali
Copy link
Collaborator Author

This PR looks good overall. But maybe we should think about how we can make Dockerfile changes like this and test container builds it in an environment that mimics an end user's and doesn't have all these custom wheels available.

Thanks @akshaysubr , you are right. I have done such testing for this and the previous PR. i.e. these builds work, even when these pre-built wheels are not available as it just reverts to source build in that case. The motivation to add these checks for pre-built wheels was to speed up internal builds. We need to do them nightly and having things build from source every time slows things down.

That being said, you brought up an interesting point. I have been discussing this with @NickGeneva recently. We need to have a robust source build instructions that mimic the docker build behavior. A lot of projects have a third_party folder to track such dependencies and we should consider adding such to Modulus. Prepared this issue to track this further: #176

@ktangsali
Copy link
Collaborator Author

/blossom-ci

@ktangsali ktangsali merged commit ccb7b64 into NVIDIA:main Sep 30, 2023
@ktangsali ktangsali deleted the ktangsali-arm-container-numcodecs-fix branch December 8, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants