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

Removing references to deprecated optuna study optimize commands from examples #1566

Merged
merged 12 commits into from
Aug 3, 2020

Conversation

ritvik1512
Copy link
Contributor

Motivation

Directly addresses issue #1394
This is the first step towards removing references to the deprecated optuna study optimize command from the attached examples.

Description of the changes

Removed mentions about optuna study optimize from the attached examples.

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.

Thanks a lot for tackling the issue. I left a comment that applies to more or less all of the examples. Could you take a look?

Also, are you able to merge/rebase the latest master and apply the same change to the recently introduced Catalyst example?

examples/allennlp/allennlp_jsonnet.py Outdated Show resolved Hide resolved
@hvy hvy added the example label Jul 29, 2020
@hvy hvy self-assigned this Jul 29, 2020
examples/catalyst_simple.py Outdated Show resolved Hide resolved
examples/allennlp/allennlp_jsonnet.py Outdated Show resolved Hide resolved
@hvy
Copy link
Member

hvy commented Jul 30, 2020

By the way, you can navigate to CircleCI's CI results from the list of jobs at the bottom. You can debug failure such as the one caused by the Catalyst examples in https://app.circleci.com/pipelines/github/optuna/optuna/1811/workflows/0fd24f83-ce8f-47be-9fc0-1a0280f4f741/jobs/50690. 🙂

@ritvik1512
Copy link
Contributor Author

ritvik1512 commented Jul 30, 2020

Not quite sure why the dependency installation failed in the doctest

@hvy
Copy link
Member

hvy commented Jul 30, 2020

Seems like a 502 error from pip. It happens sometimes when the server is under heavy load and is likely gone if retried (e.g. by another commit being pushed here or having the build re-run from the CircleCI dashboard). In any case, I don't think it's related to your changes.

@hvy
Copy link
Member

hvy commented Jul 31, 2020

Seems like you got some unrelated commits in this PR now. Could you resolved it? If you're struggling, I could create a new PR with your commits something along https://github.com/optuna/optuna/compare/master...hvy:1566-fix?expand=1

@ritvik1512
Copy link
Contributor Author

ritvik1512 commented Jul 31, 2020

Thanks @hvy I have created a new pull request (#1580) as you mentioned!

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.

Changes LGTM!

For the record, we ended up continuing on this PR as discussed in #1580 (comment).

@hvy
Copy link
Member

hvy commented Jul 31, 2020

@ritvik1512 again, thanks for addressing #1394.

It might be less interesting to work on similar issues but if you'd for instance like to continue working on the remaining files under docs, please let us know in one way or another. 🙂

@ritvik1512
Copy link
Contributor Author

Thanks, I'll stick around to help as much as I can!

@hvy hvy merged commit ad6cac5 into optuna:master Aug 3, 2020
@hvy hvy added this to the v2.1.0 milestone Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants