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

Introduce catch_warnings fixture #844

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Introduce catch_warnings fixture #844

merged 1 commit into from
Jul 29, 2022

Conversation

otaj
Copy link
Contributor

@otaj otaj commented Jul 27, 2022

What does this PR do?

Introduces catch_warnings pytest fixture, which reviewers can use to check, that no warnings other than UnderReviewWarning is emitted during the test

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?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • 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

  • Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

@otaj otaj mentioned this pull request Jul 27, 2022
@otaj otaj enabled auto-merge (squash) July 27, 2022 10:45
@shivammehta25
Copy link
Contributor

Should we also add Userwarning into the list of ignores?

from pytorch_lightning.utilities.warnings import PossibleUserWarning

warnings.simplefilter("ignore", PossibleUserWarning)
warnings.simplefilter("ignore", UserWarning)

@otaj
Copy link
Contributor Author

otaj commented Jul 27, 2022

Should we also add Userwarning into the list of ignores?

from pytorch_lightning.utilities.warnings import PossibleUserWarning

warnings.simplefilter("ignore", PossibleUserWarning)
warnings.simplefilter("ignore", UserWarning)

I'd rather not, since they are too broad, but I can be convinced :D What are some that are giving you a hard time?

@shivammehta25
Copy link
Contributor

shivammehta25 commented Jul 27, 2022

In test cases some examples were:

  • UserWarning: You passed in a `val_dataloader` but have no `validation_step`. Skipping val loop -> for GANs val_dataloader is not really useful or properly defined.
  • PossibleUserWarning: `max_epochs` was not set. Setting it to 1000 epochs. To train without an epoch limit, set `max_epochs=-1`. This one could be removed but will have to do it for each test case and I don't see any benefit of having it. But if it is too general then at least in the test cases that, I am running I can put max_epochs in the Trainer.

@otaj
Copy link
Contributor Author

otaj commented Jul 27, 2022

Regarding UserWarnings, while your usecase makes sense, I am still not convinced we should allow them in general since UserWarning is a default warning type if anyone raises a warning without specifying type. It makes sense to target the ones we know are harmless, such as the one warning about skipping val loop, on a case-by-case basis with warnings.filterwarnings("ignore", message="...", category=UserWarning)

PossibleUserWarning are similar, with the difference they are PL specific and raised with even less confidence. I'd lean on not giving them bianco check, and filter them again on a case-by-case basis, but any other opinions @Borda, @akihironitta?

@shivammehta25
Copy link
Contributor

shivammehta25 commented Jul 27, 2022

Also some more warning examples that might not lead to any major error

  • UserWarning all the test cases using seed_everything() will need to provide a seed value otherwise it throws:
    E UserWarning: No seed found, seed set to 1604981912
  • pytorch_lightning.utilities.warnings.PossibleUserWarning:` GPU available but not used. Set `accelerator` and `devices` using `Trainer(accelerator='gpu', devices=1).
  • pytorch_lightning.utilities.warnings.PossibleUserWarning: The dataloader, train_dataloader, does not have many workers which may be a bottleneck. Consider increasing the value of the `num_workers` argument` (try 20 which is the number of cpus on this machine) in the `DataLoader` init to improve performance.
  • pytorch_lightning.utilities.warnings.PossibleUserWarning: The number of training batches (1) is smaller than the logging interval Trainer(log_every_n_steps=50). Set a lower value for log_every_n_steps if you want to see logs for the training epoch.

@otaj
Copy link
Contributor Author

otaj commented Jul 27, 2022

@shivammehta007 I will go by these warnings one by one.

  • seed_everything: We should absolutely set a seed for tests so they are reproducible. I see this as a valid warning
  • GPU available but not used: This one is funny, because we just recently had a similar discussion about it in PL. What I like and find useful is to set accelerator="auto", therefore it uses best possible HW available. Not necessarily a false positive, but a nice reminder of what could be done
  • num_workers: I agree, this one is not necessarily useful for testing
  • log_every_n_steps: Again, not really useful for testing. It might be useful for debugging though to take a look at those logs (at some point in the future), can we set it to 1? Since the model is run in fast_dev_mode

I still stand by not silencing UserWarnings. PossibleUserWarning are useful hints and it can happen that a new one will appear with a new version of PyTorch, PL, or whatever other library. I'm still inclined keeping them and ignoring them on a case-by-case basis.

@shivammehta25
Copy link
Contributor

Hmm! I think that makes sense thanks! I will try setting stuff and filtering PossibleUserWarning on a case-by-case basis.

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.

Can we pls add also a usage example to the code or/and mention it in https://github.com/Lightning-AI/lightning-bolts/blob/master/.github/CONTRIBUTING.md or create a readme in the tests folder...

@otaj otaj merged commit 675b176 into master Jul 29, 2022
@otaj otaj deleted the introduce_catch_fixture branch July 29, 2022 08:33
@mergify mergify bot added the ready label Jul 29, 2022
@otaj otaj mentioned this pull request Jul 29, 2022
8 tasks
@akihironitta
Copy link
Contributor

Sorry for being late for the party, but there's already an option within pytest that we use in PL:
https://github.com/Lightning-AI/lightning/blob/2415834aaf4f215ddb0d618bf415405d670f9114/setup.cfg#L35
, and it's also possible to ignore certain warnings in some test cases as I've done before in Lightning-AI/pytorch-lightning#11755.

I guess we can still continue to use @otaj's fixture implemented in #844, but eventually, we would want to use the standardized way of handling warnings in our tests :)

@otaj
Copy link
Contributor Author

otaj commented Aug 3, 2022

Hi @akihironitta, thank you, I know about that option. However, as far as I know, that doesn't allow you to specify these fixtures on a test by test basis, and we don't want to break our CI 😂. But I fully agree, as soon as everything is using this fixture, we can and should start using the standardized way

@luca-medeiros luca-medeiros mentioned this pull request Aug 4, 2022
14 tasks
@BaruchG
Copy link
Contributor

BaruchG commented Aug 4, 2022

Hi @otaj - I'm getting the following warnings

========================================================================= warnings summary ==========================================================================
../../../miniconda3/envs/lightning/lib/python3.9/site-packages/torch/utils/tensorboard/__init__.py:4
  /home/baruch/miniconda3/envs/lightning/lib/python3.9/site-packages/torch/utils/tensorboard/__init__.py:4: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    if not hasattr(tensorboard, "__version__") or LooseVersion(

../../../miniconda3/envs/lightning/lib/python3.9/site-packages/torch/utils/tensorboard/__init__.py:6
  /home/baruch/miniconda3/envs/lightning/lib/python3.9/site-packages/torch/utils/tensorboard/__init__.py:6: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    ) < LooseVersion("1.15"):

Which looks like it is related to pytorch/pytorch#69823 and https://stackoverflow.com/questions/71666214/deprecation-warnings-distutils-and-netcdf-file. My pip freeze is

absl-py==1.2.0
aiohttp==3.8.1
aiosignal==1.2.0
ale-py==0.7.5
argon2-cffi==21.3.0
argon2-cffi-bindings==21.2.0
asttokens==2.0.5
async-timeout==4.0.2
atari-py==0.2.6
attrs==22.1.0
backcall==0.2.0
beautifulsoup4==4.11.1
black==22.6.0
bleach==5.0.1
box2d-py==2.3.8
build==0.8.0
cachetools==5.2.0
certifi @ file:///opt/conda/conda-bld/certifi_1655968806487/work/certifi
cffi==1.15.1
cfgv==3.3.1
charset-normalizer==2.1.0
check-manifest==0.48
click==8.1.3
cloudpickle==1.6.0
codecov==2.1.12
coloredlogs==15.0.1
commonmark==0.9.1
coverage==6.4.2
cryptography==37.0.4
cycler==0.11.0
debugpy==1.6.2
decorator==5.1.1
defusedxml==0.7.1
distlib==0.3.5
dlib==19.24.0
docker-pycreds==0.4.0
docstring-parser==0.14.1
docutils==0.19
entrypoints==0.4
executing==0.9.1
fastjsonschema==2.16.1
filelock==3.7.1
flake8==5.0.2
flatbuffers==2.0
fonttools==4.34.4
frozenlist==1.3.0
fsspec==2022.7.1
gitdb==4.0.9
GitPython==3.1.27
google-auth==2.9.1
google-auth-oauthlib==0.4.6
GPUtil==1.4.0
grpcio==1.47.0
gym==0.19.0
humanfriendly==10.0
identify==2.5.2
idna==3.3
imageio==2.21.0
importlib-metadata==4.12.0
importlib-resources==5.9.0
iniconfig==1.1.1
ipykernel==6.15.1
ipython==8.4.0
ipython-genutils==0.2.0
ipywidgets==7.7.1
isort==5.10.1
jedi==0.18.1
jeepney==0.8.0
Jinja2==3.1.2
joblib==1.1.0
jsonargparse==4.12.0
jsonschema==4.9.0
jupyter==1.0.0
jupyter-client==7.3.4
jupyter-console==6.4.4
jupyter-core==4.11.1
jupyterlab-pygments==0.2.2
jupyterlab-widgets==1.1.1
keyring==23.7.0
kiwisolver==1.4.4
Markdown==3.4.1
MarkupSafe==2.1.1
matplotlib==3.5.2
matplotlib-inline==0.1.3
mccabe==0.7.0
merge-args==0.1.4
mistune==0.8.4
mpmath==1.2.1
multidict==6.0.2
mypy==0.971
mypy-extensions==0.4.3
nbclient==0.6.6
nbconvert==6.5.0
nbformat==5.4.0
nest-asyncio==1.5.5
networkx==2.8.5
nodeenv==1.7.0
notebook==6.4.12
numpy==1.23.1
oauthlib==3.2.0
onnx==1.10.1
onnxruntime==1.12.0
opencv-python==4.6.0.66
opencv-python-headless==4.6.0.66
packaging==21.3
pandas==1.4.3
pandocfilters==1.5.0
parso==0.8.3
pathspec==0.9.0
pathtools==0.1.2
pep517==0.13.0
pexpect==4.8.0
pickleshare==0.7.5
Pillow==9.2.0
pkginfo==1.8.3
platformdirs==2.5.2
pluggy==1.0.0
pre-commit==2.20.0
progressbar2==4.0.0
prometheus-client==0.14.1
promise==2.3
prompt-toolkit==3.0.30
protobuf==3.19.4
psutil==5.9.1
ptyprocess==0.7.0
pure-eval==0.2.2
py==1.11.0
pyasn1==0.4.8
pyasn1-modules==0.2.8
pycodestyle==2.9.0
pycparser==2.21
pydantic==1.9.1
pyDeprecate==0.3.2
pyflakes==2.5.0
Pygments==2.12.0
pyparsing==3.0.9
pyrsistent==0.18.1
pytest==7.1.2
pytest-cov==3.0.0
python-dateutil==2.8.2
python-utils==3.3.3
pytorch-lightning==1.6.5
pytz==2022.1
PyWavelets==1.3.0
PyYAML==6.0
pyzmq==23.2.0
qtconsole==5.3.1
QtPy==2.1.0
readme-renderer==35.0
requests==2.28.1
requests-oauthlib==1.3.1
requests-toolbelt==0.9.1
rfc3986==2.0.0
rich==12.5.1
rsa==4.9
scikit-image==0.19.3
scikit-learn==1.1.1
scipy==1.9.0
SecretStorage==3.3.2
Send2Trash==1.8.0
sentry-sdk==1.9.0
setproctitle==1.3.0
shortuuid==1.0.9
six==1.16.0
smmap==5.0.0
soupsieve==2.3.2.post1
sparseml==0.12.2
sparsezoo==0.12.1
stack-data==0.3.0
sympy==1.10.1
tensorboard==2.9.1
tensorboard-data-server==0.6.1
tensorboard-plugin-wit==1.8.1
terminado==0.15.0
threadpoolctl==3.1.0
tifffile==2022.7.31
tinycss2==1.1.1
toml==0.10.2
tomli==2.0.1
toposort==1.7
torch==1.12.0
torchmetrics==0.9.3
torchvision==0.13.0
tornado==6.2
tqdm==4.64.0
traitlets==5.3.0
twine==4.0.1
typing_extensions==4.3.0
urllib3==1.26.11
virtualenv==20.16.2
wandb==0.12.21
wcwidth==0.2.5
webencodings==0.5.1
Werkzeug==2.2.1
widgetsnbextension==3.6.1
yarl==1.8.0
zipp==3.8.1

Everything was installed by using the requirements.txt provided in bolts, I'm using Python 3.9.12. This seems like a broader issue than lightning-bolts, how should I deal with this?

@akihironitta
Copy link
Contributor

Hi @BaruchG, just created an issue #853. Let's continue to discuss it in the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants