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

Docker images for v2 #841

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

JacksonBurns
Copy link
Member

This PR:

  • add automatic building of PyPI and Docker images back to the CI, which we had forgotten to add during the initial release of v2
  • updates the usage docs to be more helpful and concise
  • updates the dockerfile to avoid installing gpu pytorch when the container only runs cpu

I have already built and pushed a v2.0.0 image manually to Dockerhub. In the future we will have builds continuously made - when we want to do a release we just pull the built image, tag it, and push it back.

@kevingreenman kevingreenman added this to the v2.0.1 milestone Apr 25, 2024
Copy link
Member

@kevingreenman kevingreenman left a comment

Choose a reason for hiding this comment

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

@JacksonBurns thanks for the PR! I just have a couple questions before we merge

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@JacksonBurns
Copy link
Member Author

ugh.

the latest macos runners (the apple silicon ones) dont support the default DDP strategy in lightning and are not accessible from our runners which causes errors when lightning attempts to use it when accelerators="auto".

@kevingreenman I guess we should resolve the test failures in a separate PR before this one.

@kevingreenman
Copy link
Member

did this just change today? I thought all the tests were passing earlier today

@JacksonBurns
Copy link
Member Author

Yeah this only just changed, the CI is failing now too: https://github.com/chemprop/chemprop/actions/runs/8845256777

@JacksonBurns
Copy link
Member Author

It was probably torch 2.3, which just came out: https://pytorch.org/blog/pytorch2-3/

@JacksonBurns
Copy link
Member Author

finally found someone else reporting the error: https://discuss.pytorch.org/t/mps-back-end-out-of-memory-on-github-action/189773/2

pytorch knows about it and has closed the issue tracking it: pytorch/pytorch#111449 (comment)

and yet this issue remains open showing that this is not resolved: actions/runner-images#9254

solution is to not run on the latest macos... sad

see: chemprop#841 (comment)

thank god apple made their own gpu, what even are standards anyway
@JacksonBurns
Copy link
Member Author

@kevingreenman switching back to macos13 which uses intel chips works. If you are ok with this we can merge as-is to get CI passing and re-open #776 to track adding apple silicon runners back

@kevingreenman
Copy link
Member

@JacksonBurns that sounds good to me. It looks like the status checks are still showing the macos-latest tests as being expected/required, but if I click on "Details" to see the more in-depth status of any of the tests, it shows that everything is complete, and macos-latest is not present on that page. Are these still showing up on this PR because they were required when the PR was opened (or maybe because they're required in the target branch)? If so, I think we can use the "bypass branch protections" option.

@JacksonBurns
Copy link
Member Author

yes, the branch protection still expects macos latest. With your approval here, I have now changed this rule to instead look for macos13 and you should be able to merge this normally.

@kevingreenman kevingreenman merged commit be8340d into chemprop:main Apr 30, 2024
13 checks passed
@kevingreenman
Copy link
Member

thanks!

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.

None yet

2 participants