Skip to content

Conversation

@GabrielePicco
Copy link
Contributor

@GabrielePicco GabrielePicco commented Jun 18, 2021

Older version of the packaging module cause an error when saving model checkpoint.

The error is in the atomic_save: if Version(torch.__version__).release[:3] == (1, 6, 0):

For old version of packaging (e.g., 16.8) the call produce the following errror:

AttributeError: 'Version' object has no attribute 'release'

What does this PR do?

Specify packaging version to be grater or equal to 17.0 in order to avoid bug in the atomic checkpoints save

I tested packing version from 17.0 to 20.9 (latest at the time of writing) and the bug is fixed

How to reproduce the bug

  1. pip install packaging==16.8
  2. Run any training that save a checkpoint

or reproducing the call at line: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/utilities/cloud_io.py#L61

  1. pip install packaging==16.8
  2. python
    • import torch; from packaging.version import Version
    • Version(torch.version).release[:3]

Older version of packaging causes an error when saving model checkpoint.

The error is in the atomic_save: `if Version(torch.__version__).release[:3] == (1, 6, 0):`

For old version of packaging (e.g., 16.8) the call produce the following errror:

`AttributeError: 'Version' object has no attribute 'release'`
@carmocca carmocca added bug Something isn't working ci Continuous Integration labels Jun 18, 2021
@carmocca carmocca added this to the v1.3.x milestone Jun 18, 2021
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #8030 (3f13d37) into master (0d6dfd4) will decrease coverage by 5%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #8030    +/-   ##
=======================================
- Coverage      92%     87%    -5%     
=======================================
  Files         210     210            
  Lines       13576   13576            
=======================================
- Hits        12449   11807   -642     
- Misses       1127    1769   +642     

Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

@carmocca carmocca enabled auto-merge (squash) June 18, 2021 17:54
@carmocca carmocca added the ready PRs ready to be merged label Jun 18, 2021
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

ship it!

@GabrielePicco
Copy link
Contributor Author

@awaelchli @Borda @carmocca @justusschock any suggestion of why some tests is failing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci Continuous Integration ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants