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

stable, dev PyTorch in Dockerfile and conda gh actions #3074

Merged
merged 24 commits into from
Sep 17, 2020
Merged

stable, dev PyTorch in Dockerfile and conda gh actions #3074

merged 24 commits into from
Sep 17, 2020

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Aug 20, 2020

What does this PR do?

Add Docker image installed with pytorch nightly, PyTorch & Conda gh action tested with pytorch nightly

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@mergify mergify bot requested a review from a team August 20, 2020 13:08
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #3074 into master will decrease coverage by 2%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3074    +/-   ##
=======================================
- Coverage      88%     86%    -2%     
=======================================
  Files         109     109            
  Lines        8436    8043   -393     
=======================================
- Hits         7409    6916   -493     
- Misses       1027    1127   +100     

@Borda Borda added this to the 0.9.x milestone Aug 20, 2020
@mergify mergify bot requested a review from a team August 21, 2020 17:17
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls, update the cuda based image such that the 1.7/nightmli is just an argument so we avoid maintaining too many images...

@mergify mergify bot requested a review from a team August 21, 2020 17:19
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 22, 2020

@Borda torch is not installed as locked version in build-cuda github action.
https://github.com/PyTorchLightning/pytorch-lightning/runs/1013619899?check_suite_focus=true
also, i think installing different torch versions with conda might be easier than installing with pip

Screen Shot 2020-08-22 at 14 29 38

@Borda Borda added feature Is an improvement or enhancement ci Continuous Integration labels Aug 24, 2020
@Borda
Copy link
Member

Borda commented Aug 24, 2020

maybe but in Conda env we are running only CPU tests compare to the CUDA-pip image we run in Drone with the full suite case

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 24, 2020

Found out pytorch CUDA image itself is also using conda, so go with it right now.
would you like to change to pip?

@Borda
Copy link
Member

Borda commented Aug 24, 2020

Found out pytorch CUDA image itself is also using conda, so go with it right now.
would you like to change to pip?

Well but is conda activated by default?

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 24, 2020

Found out pytorch CUDA image itself is also using conda, so go with it right now.
would you like to change to pip?

Well but is conda activated by default?

yes

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 26, 2020

for GPU testing with stable and dev PyTorch on Drone, shall we do testing in parallel or one after another?
I go for one after another
WDYT?

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 27, 2020

@Borda I have noticed that PyTorch & Conda github actions is installing gpu version of pytorch while we are not testing with gpu. Is this for some reasons or shall i add cpuonly package in environment.yml to save a few pkg installing minutes?

@Borda Borda self-assigned this Aug 27, 2020
@ydcjeff ydcjeff requested review from Borda and removed request for a team September 2, 2020 14:49
@mergify mergify bot requested a review from a team September 2, 2020 14:49
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 3, 2020

@Borda mind review this so far?
so that i can proceed #3074 (comment) and #3074 (comment)

@Borda
Copy link
Member

Borda commented Sep 3, 2020

btw, is it still wip?

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 3, 2020

btw, is it still wip?

@Borda I haven't worked on Drone CI for GPU testing, so can be said wip
waiting for review of current base-cuda Dokerfile, so that i can continue

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 4, 2020

@Borda
Copy link
Member

Borda commented Sep 4, 2020

@Borda do I need to change this docker image?
https://github.com/PyTorchLightning/pytorch-lightning/blob/64ee25ed9cd1d44bb276b61683bbdfee0b00ff9d/.drone.yml#L23

you cannot until it is build for the first time and push to the docker hub...

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 4, 2020

@Borda do I need to change this docker image?
https://github.com/PyTorchLightning/pytorch-lightning/blob/64ee25ed9cd1d44bb276b61683bbdfee0b00ff9d/.drone.yml#L23

you cannot until it is build for the first time and push to the docker hub...

Ok then, shall we build the docker from this PR without merging into master?
OR build the docker from master and make another follow PR of this?

@ydcjeff ydcjeff changed the title [WIP] build, test with stable and dev PyTorch stable, dev PyTorch in Dockerfile and gh actions Sep 4, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 4, 2020

@Borda ready to review

@Borda Borda requested a review from williamFalcon September 17, 2020 13:06
@Borda Borda added the ready PRs ready to be merged label Sep 17, 2020
@Borda
Copy link
Member

Borda commented Sep 17, 2020

@ydcjeff seems that tests for pt 1.7 are failing so I would recommend to keep the CI ready and just comment for now...
then make another PR which would fix the failing test and uncomment the testing

Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM, great job

Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

All good. Nice work!

@Borda Borda merged commit 8be79a9 into Lightning-AI:master Sep 17, 2020
@Borda Borda mentioned this pull request Sep 17, 2020
2 tasks
@ydcjeff ydcjeff deleted the nightly branch September 18, 2020 02:22
@Borda Borda mentioned this pull request Sep 18, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants