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

Upgrade pytorch requirement to >=1.2.0 #1565

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Upgrade pytorch requirement to >=1.2.0 #1565

merged 1 commit into from
Apr 6, 2020

Conversation

vincentschen
Copy link
Member

@vincentschen vincentschen commented Apr 5, 2020

Description of proposed changes

  • Update dev + setup.py requirements for torch
  • Use native torch.utils.tensorboard and remove tensorboardX dep
  • Moved lr_scheduler.step() after optimizer.step()
  • Replace deprecated uses of .byte() with .bool()

Related issue(s)

Fixes #1558

Test plan

  • All unit tests pass with torch==1.2.0, 1.3.0, 1.4.0
  • Updated Slicing convergence test for longer fit.
  • tox -e {complex,spark} passes with torch==1.2.0, 1.3.0, 1.4.0

Checklist

Need help on these? Just ask!

  • I have read the CONTRIBUTING document.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have run tox -e complex and/or tox -e spark if appropriate.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Apr 5, 2020

Codecov Report

Merging #1565 into master will not change coverage by %.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1565   +/-   ##
=======================================
  Coverage   97.13%   97.13%           
=======================================
  Files          56       56           
  Lines        2091     2091           
  Branches      342      342           
=======================================
  Hits         2031     2031           
  Misses         31       31           
  Partials       29       29           
Impacted Files Coverage Δ
...ssification/training/loggers/tensorboard_writer.py 100.00% <100.00%> (ø)
snorkel/classification/training/trainer.py 89.83% <100.00%> (ø)
snorkel/labeling/model/label_model.py 95.54% <100.00%> (ø)
snorkel/slicing/utils.py 94.66% <100.00%> (ø)

requirements.txt Outdated
@@ -18,15 +18,14 @@ tqdm>=4.33.0,<5.0.0

# Internal models
scikit-learn>=0.20.2,<0.22.0
torch>=1.1.0,<1.2.0
torch>=1.4.0,<2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Is this incompatible with 1.2 and 1.3?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, all tests pass with the older versions 1.2.0 and 1.3.0.
relaxing the requirement to torch>=1.2.0,<2.0.0

@vincentschen vincentschen changed the title Upgrade pytorch requirement to 1.4.0 Upgrade pytorch requirement to >=1.2.0 Apr 5, 2020
@lukehsiao lukehsiao mentioned this pull request Apr 5, 2020
Copy link
Member

@henryre henryre left a comment

Choose a reason for hiding this comment

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

just one question, otherwise lgtm!!

@@ -41,9 +41,9 @@
"pandas>=0.25.0,<0.26.0",
"tqdm>=4.33.0,<5.0.0",
"scikit-learn>=0.20.2,<0.22.0",
"torch>=1.1.0,<1.2.0",
"torch>=1.2.0,<2.0.0",
"tensorboard>=1.14.0,<2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

how did we decide on this range?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got an import error that looked like this:

ImportError: TensorBoard logging requires TensorBoard with Python summary writer installed. This should be available in 1.14 or above

@henryre
Copy link
Member

henryre commented Apr 6, 2020

@vincentschen can we update the Test Plan with details from the multiple version tests?

@vincentschen
Copy link
Member Author

@vincentschen can we update the Test Plan with details from the multiple version tests?

Yep! Naively, pinned the version to each minor release and ran all unit/integration tests.

@vincentschen vincentschen merged commit 1adc7a8 into master Apr 6, 2020
@vincentschen vincentschen deleted the vsc-torch1.4 branch April 6, 2020 19:03
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.

Support Pytorch > 1.1.0
3 participants