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

refactor and simplify optional imports #2877

Closed
wants to merge 20 commits into from
Closed

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Aug 8, 2020

What does this PR do?

Fixes #2266

refactors code duplication for recurring optional imports like apex, native amp, xla, etc.
it's also backward compatible with e.g. bolts that needs APEX_AVAILABLE etc.

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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • 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

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 🙃

@mergify mergify bot requested a review from a team August 8, 2020 03:11
@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

Merging #2877 into master will increase coverage by 4%.
The diff coverage is 94%.

@@           Coverage Diff           @@
##           master   #2877    +/-   ##
=======================================
+ Coverage      86%     90%    +4%     
=======================================
  Files          83      80     -3     
  Lines        7861    7185   -676     
=======================================
- Hits         6745    6445   -300     
+ Misses       1116     740   -376     

@awaelchli awaelchli added the feature Is an improvement or enhancement label Aug 8, 2020
@Borda Borda changed the title [WIP] refactor and simplify optional imports (proposal) [WIP, blocked by #2865] refactor and simplify optional imports (proposal) Aug 8, 2020
@Borda Borda changed the title [WIP, blocked by #2865] refactor and simplify optional imports (proposal) [WIP] refactor and simplify optional imports (proposal) Aug 8, 2020
@Borda
Copy link
Member

Borda commented Aug 8, 2020

mind update after #2865

@awaelchli awaelchli added this to the 0.9.x milestone Aug 8, 2020
@awaelchli
Copy link
Contributor Author

@Borda adding this to 0.9.x milestone just in case I have a typo, I don't want it to be released :)

@pep8speaks
Copy link

pep8speaks commented Aug 8, 2020

Hello @awaelchli! Thanks for updating this PR.

Line 11:61: W504 line break after binary operator
Line 16:59: W504 line break after binary operator

Comment last updated at 2020-08-24 18:18:41 UTC

try:
from test_tube import HyperOptArgumentParser
except ImportError:
# TODO: this should be discussed and moved out of this package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it nowhere used here, so it seems we can easily remove it.

Comment on lines -16 to -34
try:
from torch.utils.data import IterableDataset
ITERABLE_DATASET_EXISTS = True
except ImportError:
ITERABLE_DATASET_EXISTS = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this, because iterable dataset is part of torch since 1.2 and we support torch >= 1.3

@awaelchli awaelchli changed the title [WIP] refactor and simplify optional imports (proposal) [WIP] refactor and simplify optional imports Aug 8, 2020
@awaelchli awaelchli modified the milestones: 0.9.x, 1.0.0 Aug 8, 2020
@ananyahjha93 ananyahjha93 self-requested a review August 8, 2020 16:05
@Borda Borda removed this from the 1.0.0 milestone Aug 20, 2020
@Borda Borda added this to the 0.9.x milestone Aug 20, 2020
@awaelchli awaelchli changed the title [WIP] refactor and simplify optional imports refactor and simplify optional imports Aug 24, 2020
@awaelchli awaelchli closed this Sep 12, 2020
@awaelchli awaelchli deleted the optional-imports-2 branch September 12, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Exception Handling
3 participants