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.

Sign up for GitHub

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

Jump to bottom

Use packaging to check the library version #1610

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Use packaging to check the library version #1610

merged 3 commits into from
Aug 11, 2020

Conversation

VamshiTeja
Copy link
Contributor

Motivation

Use packaging to check the library version
Related #1589

Description of the changes

Update the version checking in the following files to use packaging:

Files which use distutils:

  • optuna/dashboard.py
  • optuna/visualization/_plotly_imports.py

Files which use pkg_resources:

  • examples/allennlp/allennlp_jsonnet.py
  • examples/allennlp/allennlp_simple.py
  • examples/chainer_simple.py
  • examples/pruning/chainer_integration.py
  • examples/pytorch_lightning_simple.py
  • examples/tensorflow_eager_simple.py
  • tests/integration_tests/test_tfkeras.py

@github-actions github-actions bot added the optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. label Aug 9, 2020
@HideakiImamura HideakiImamura self-assigned this Aug 11, 2020
@HideakiImamura HideakiImamura added code-fix Change that does not change the behavior, such as code refactoring. and removed optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. labels Aug 11, 2020
@HideakiImamura
Copy link
Member

HideakiImamura commented Aug 11, 2020 •
edited

Thanks for the PR! The changes look good to me. By the way, the recent sphinx's update broken some CI tests, but it is fixed in #1613. Could you merge the master branch to pass the CI?

@github-actions github-actions bot added the optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. label Aug 11, 2020
@VamshiTeja
Copy link
Contributor Author

Thanks for the PR! The changes look good to me. By the way, the recent sphinx's update broken some CI tests, but it is fixed in #1613. Could you merge the master branch to pass the CI?

@HideakiImamura Thanks for the review. I've fixed CI tests!

toshihikoyanase
toshihikoyanase approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment •
edited

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution!
In some files, packaging is imported in the group of the standard libraries, but it is actually a third-party library. I don't think it is a critical problem, and I'll approve this PR. If you do not work on them in this PR, I'll make a follow-up PR after merging this PR.

@VamshiTeja
Copy link
Contributor Author

In some files, packaging is imported in the group of the standard libraries, but it is actually a third-party library. I don't think it is a critical problem, and I'll approve this PR. If you do not work on them in this PR, I'll make a follow-up PR after merging this PR.

Thanks for the review. Let me fix it.

toshihikoyanase
toshihikoyanase approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my comment. LGTM!

hvy
hvy approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

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

That was really fast. Thanks for the PR and the changes LGTM!

Let me merge this PR as it has 2 approvals. I believe @HideakiImamura is fine with that 🙂

@hvy hvy merged commit d3f7202 into optuna:master Aug 11, 2020
@hvy hvy added this to the v2.1.0 milestone Aug 11, 2020
@hvy hvy mentioned this pull request Aug 11, 2020
Merged
@toshihikoyanase toshihikoyanase mentioned this pull request Aug 11, 2020
Closed
9 tasks
@hvy hvy mentioned this pull request Aug 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toshihikoyanase toshihikoyanase toshihikoyanase approved these changes

@hvy hvy hvy approved these changes

Assignees

@HideakiImamura HideakiImamura

Labels
code-fix Change that does not change the behavior, such as code refactoring. optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@VamshiTeja @HideakiImamura @toshihikoyanase @hvy