Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

update pystiche dependency #642

Merged
merged 4 commits into from
Aug 7, 2021

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Aug 7, 2021

What does this PR do?

This upgrades a couple of things regarding the pystiche dependency:

  • pystiche==1.0.0 was released and now follows semantic versioning. Thus, It is reasonable to pin it to the current stable release.
  • pystiche.ops is deprecated in favor of pystiche.loss. Although everything should work as expected as is, without change the code will now emit deprecation warnings. All occurrences of pystiche.ops were replaced.
  • pystiche's default branch was changed from master to main. A link was updated to reflect this.
  • pystiche should be written with a lower p. All occurrences in the documentation that did not do this were updated.

Fixes # (issue)

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 🙃

@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #642 (9d5a83e) into master (6483d1c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   90.63%   90.63%   -0.01%     
==========================================
  Files         173      173              
  Lines        9182     9179       -3     
==========================================
- Hits         8322     8319       -3     
  Misses        860      860              
Flag Coverage Δ
gpu ?
pytest ?
unittests 90.63% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flash/image/style_transfer/model.py 94.02% <100.00%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6483d1c...9d5a83e. Read the comment docs.

@pmeier
Copy link
Contributor Author

pmeier commented Aug 7, 2021

Two problems here:

  • Some workflows fail, because they can't import stuff from pystiche>=1.0.0 although they never install any version explicitly. This tells me, that they pull in an older version from cache. I'm not in a position to fix that.
  • JIT tests are failing, because parts of the pystiche.loss.PerceptualLoss are no longer torch.jit.trace'able. This worked before, but only by chance. I'm wondering, why do you test for losses being able to be traced? AFAIK, torch.jit is used for efficient deployment of models for which the loss that they were trained on is irrelevant.

Copy link
Collaborator

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

LGTM. @pmeier I fixed the issues:

  • It wasn't pystiche getting cached, the error was just cause by the use of pystiche components in the type annotation despite pystiche not being installed
  • Regarding JIT, we don't try to jit the loss, only the forward. The issue seems to be that JIT.trace first recurses over all callable attributes of the model to see if they have been marked for export. When it hits loss.encoder, that throws a runtime error which is then not caught. I think the Multiple enocder attributes error in pystiche could be replaced with an AttributeError rather than a RuntimeError in order to fix this. For now I have just removed the loss_fn and perceptual_loss attributes before jitting in the test.

@ethanwharris ethanwharris merged commit 60d2733 into Lightning-Universe:master Aug 7, 2021
@pmeier pmeier deleted the upgrade-pystiche branch August 8, 2021 09:19
@pmeier
Copy link
Contributor Author

pmeier commented Aug 8, 2021

@ethanwharris

Thanks for the input.

It wasn't pystiche getting cached, the error was just cause by the use of pystiche components in the type annotation despite pystiche not being installed

Oh, I see. Wouldn't it make a lot more sense to quote the annotations instead of providing mock modules? That way there is no runtime effect in case pystiche is not available and if it is available mypy treats it the same as an unqouted annotation.

Regarding JIT, we don't try to jit the loss, only the forward. The issue seems to be that JIT.trace first recurses over all callable attributes of the model to see if they have been marked for export. When it hits loss.encoder, that throws a runtime error which is then not caught. I think the Multiple enocder attributes error in pystiche could be replaced with an AttributeError rather than a RuntimeError in order to fix this. For now I have just removed the loss_fn and perceptual_loss attributes before jitting in the test.

That is a valid point. I'll go and fix this there and send a new patch here.

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

Successfully merging this pull request may close these issues.

2 participants