Add support for Lightning 1.8 + Fixes for the CI #1479
Conversation
krshrimali
requested review from
ethanwharris,
Borda,
tchaton,
justusschock and
kaushikb11
as code owners
| @@ -128,7 +127,7 @@ jobs: | |||
| if: contains( matrix.topic , 'serve' ) | |||
| run: | | |||
| sudo apt-get install libsndfile1 | |||
| pip install '.[all,audio]' icevision effdet --upgrade | |||
| pip install '.[all,audio]' icevision sahi==0.8.19 effdet --upgrade | |||
There was a problem hiding this comment.
why icevision, sahi, effdet are not part of the requirements?
There was a problem hiding this comment.
They are. We don't need these modules for serve though, but for serve testing we need them. Hence explicit installation. @Borda
There was a problem hiding this comment.
| pip install '.[all,audio]' icevision sahi==0.8.19 effdet --upgrade | |
| pip install '.[all,audio,serve]' --upgrade |
There was a problem hiding this comment.
but still why all does not include all?
There was a problem hiding this comment.
uhmm, because we only need icevision, effdet for the serve tests. And these are in requirements/datatype_image_extras.txt (which has a lot more than we need for serve tests).
So basically:
icevision, effdetare not needed for serve in Lightning Flash.icevision, effdetare only needed for "serve tests" in Flash.
Hence the explicit mention of sahi, icevision, effdet (sahi because we need to pin it).
Does that help, @Borda ? Please let me know if you have any questions or suggestions.
There was a problem hiding this comment.
still, sahi, icevision, effdet shall be in a requirement file for reproducibility in case you would need to restrict versions
There was a problem hiding this comment.
And we already use this^ for image.
And this is only temporary, once icevision is fixed for the latest sahi release, this will go.
There was a problem hiding this comment.
let's do these requirements cleaning in another PR, and lets link this discussion 🦦
There was a problem hiding this comment.
Yes! ^ From an off-line discussion with @Borda, I fully agree that we should make reproducible environments for users to have the same testing environment as the CI. Nobody will read the CI dependencies to figure out why it fails in the CI.
So, we'll do something similar to what TorchMetrics has: https://github.com/Lightning-AI/metrics/tree/master/requirements
I'll make a PR and link the discussion here. Thank you, @Borda!!
for more information, see https://pre-commit.ci
…g-flash into debug-lightning-18
Codecov ReportBase: 71.32% // Head: 86.33% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1479 +/- ##
===========================================
+ Coverage 71.32% 86.33% +15.00%
===========================================
Files 292 292
Lines 13079 13158 +79
===========================================
+ Hits 9329 11360 +2031
+ Misses 3750 1798 -1952
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
for more information, see https://pre-commit.ci
…g-flash into debug-lightning-18
This reverts commit 30deeb6.
…g-flash into debug-lightning-18
for more information, see https://pre-commit.ci
krshrimali
changed the title
What does this PR do?
Fixes #1481 and adds support for Lightning 1.8
Before submitting
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 🙃