Skip to content

Fix(Topofit): Fixed Docker Image error #4

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gaiborjosue
Copy link
Collaborator

This PR will successfully close #3 @hvgazula

@gaiborjosue
Copy link
Collaborator Author

gaiborjosue commented Aug 2, 2023

@hvgazula The action will fail on my end because I don't have docker credentials set up in the repo. However, the image was built successfully. In other words, the only step that failed on my end was pushing the image. So, this will indeed solve #3

@hvgazula
Copy link
Collaborator

hvgazula commented Aug 2, 2023

@gaiborjosue I too got it to work with 0.6.0 myself. But I am trying to stay true to the user's setup. I spoke to @WilliamAshbee yesterday and he was pretty sure he did not have any issues with building an image with the Dockerfile as is. That is, with 0.0.8.

@gaiborjosue
Copy link
Collaborator Author

gaiborjosue commented Aug 2, 2023

@hvgazula, I see. I will take a look at that.

@hvgazula hvgazula marked this pull request as draft August 2, 2023 14:54
@WilliamAshbee
Copy link

I will see if i can get topofit to build again on my end. maybe i have some local changes or maybe something was deprecated. at one point topofit was pretty smooth build.

@gaiborjosue
Copy link
Collaborator Author

Hello @WilliamAshbee, that would be helpful. I tried installing surfa==0.0.8 locally, and it did not work either.

@WilliamAshbee
Copy link

It looks like surfa==0.0.8 is not in pypi anymore (not sure how that happened), but I removed this line from the environment.yml and did a 'RUN pip install surfa' in the docker file and the build finishes. @hvgazula and I discussed this earlier today.

@gaiborjosue
Copy link
Collaborator Author

gaiborjosue commented Aug 2, 2023 via email

@hvgazula
Copy link
Collaborator

hvgazula commented Aug 2, 2023

It looks like surfa==0.0.8 is not in pypi anymore (not sure how that happened), but I removed this line from the environment.yml and did a 'RUN pip install surfa' in the docker file and the build finishes. @hvgazula and I discussed this earlier today.

Thanks @WilliamAshbee. Is this your final go-ahead (aka did you get to run some preliminary tests to ensure that the results are just the same)?

@hvgazula
Copy link
Collaborator

hvgazula commented Aug 2, 2023

Another option that I just realized is to upload your earliest image (with 0.0.8) and we will use that in the zoo. what do you think @WilliamAshbee?

@WilliamAshbee
Copy link

@hvgazula I never pushed to dockerhub unfortunately. Our local storage doesn't usually last more than a few weeks. Sorry, I would think the most recent version is surfa would work. If you would like I can perform some testing.

@hvgazula
Copy link
Collaborator

hvgazula commented Aug 2, 2023

@hvgazula I never pushed to dockerhub unfortunately. Our local storage doesn't usually last more than a few weeks. Sorry, I would think the most recent version is surfa would work. If you would like I can perform some testing.

I thought that's what you wanted to do. But, again only if you have time. If not, we will proceed with it as is (with the new modification) with no guarantees provided for future results. 😄

@WilliamAshbee
Copy link

I would proceed as is, and in parallel i will test it.

@gaiborjosue gaiborjosue force-pushed the main branch 2 times, most recently from fd09cb1 to 567e2fc Compare August 3, 2023 17:25
@WilliamAshbee
Copy link

I have updated the dockerfile. The versions of all dependencies has remained the same. Had to use anaconda instead of pip for torch-scatter. Had to compile surfa from source, but is same version as tested version. Had @MiaLinlinLu verify that it matches the review paper's results, so it reproduces results on the test set. The github fork has the updated dockerfile, yml, and surfa source (small). https://github.com/neuroneural/topofit_fork

@hvgazula
Copy link
Collaborator

hvgazula commented Aug 4, 2023 via email

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.

Topofit/Surfa Docker image build error
4 participants