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

Prune deprecated: classification and regression #806

Merged
merged 20 commits into from
Feb 5, 2022

Conversation

ashutoshml
Copy link
Contributor

@ashutoshml ashutoshml commented Jan 27, 2022

What does this PR do?

Fixes part of #771

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 to update the docs?
  • Did you write any new necessary tests?

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 🙃

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #806 (5ea82b7) into master (8e9eb8e) will increase coverage by 1%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #806   +/-   ##
=====================================
+ Coverage      95%    95%   +1%     
=====================================
  Files         165    163    -2     
  Lines        6776   6710   -66     
=====================================
- Hits         6410   6384   -26     
+ Misses        366    326   -40     

@ashutoshml
Copy link
Contributor Author

Failing integration test for python 3.6. Not sure how to resolve this.

@ashutoshml ashutoshml mentioned this pull request Jan 27, 2022
5 tasks
@Borda Borda added the enhancement New feature or request label Jan 27, 2022
@Borda Borda added this to the v0.8 milestone Jan 27, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot added the ready label Jan 27, 2022
@Borda Borda enabled auto-merge (squash) January 27, 2022 10:55
auto-merge was automatically disabled January 27, 2022 20:25

Pull Request is not mergeable

@mergify mergify bot removed the ready label Jan 27, 2022
@Borda
Copy link
Member

Borda commented Jan 27, 2022

@SkafteNicki seems we have the same issue with PL compatibility here too, as F1 was one of the very legacy metrics first implemented in PL itself... 🐰

so shall we break the compatibility with PL v1.3 or preserve this burden till TM v1.0?
cc: @PyTorchLightning/core-metrics @PyTorchLightning/core-lightning

@mergify mergify bot added the ready label Jan 27, 2022
@SkafteNicki
Copy link
Member

@Borda this is a though one, but I am in favour of breaking backwards-compatibility with lightning. We need first and foremost to think of metrics as its own package which does not depend on lightning. In practise I do not know how many actually uses metrics with base torch and how many uses it with lightning.
However, I would also be more comfortable with this breaking change if lightning v1.6 is out before v0.8 of metrics. This would mean that TM v0.8 is compatible with two major versions of lightning v1.5 and v1.6. If you are using lightning <v1.5 we recommend sticking with TM v0.7.

@Borda
Copy link
Member

Borda commented Jan 28, 2022

However, I would also be more comfortable with this breaking change if lightning v1.6 is out before v0.8 of metrics. This would mean that TM v0.8 is compatible with two major versions of lightning v1.5 and v1.6. If you are using lightning <v1.5 we recommend sticking with TM v0.7.

we have this testing already inside EcoCI - https://github.com/PyTorchLightning/ecosystem-ci/blob/main/configs/PyTorchLightning/metrics_pl-develop.yaml

@awaelchli
Copy link
Contributor

However, I would also be more comfortable with this breaking change if lightning v1.6 is out before v0.8 of metrics. This would mean that TM v0.8 is compatible with two major versions of lightning v1.5 and v1.6. If you are using lightning <v1.5 we recommend sticking with TM v0.7.

+1 for this approach

@Borda Borda enabled auto-merge (squash) February 4, 2022 23:53
@Borda Borda self-assigned this Feb 4, 2022
@ashutoshml
Copy link
Contributor Author

@Borda: Any updates on the final approach for this issue?

@Borda
Copy link
Member

Borda commented Feb 5, 2022

@Borda: Any updates on the final approach for this issue?

it is going to merge as soon as all test passes :)

@Borda Borda merged commit 8ee6523 into Lightning-AI:master Feb 5, 2022
@ashutoshml ashutoshml deleted the pruneclassification branch February 5, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants