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

Add constraint null checking in graph components #972

Closed
wants to merge 2 commits into from

Conversation

hrntsm
Copy link
Collaborator

@hrntsm hrntsm commented Sep 26, 2024

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

What does this implement/fix? Explain your changes.

In the same study, when a trial with constraints and a trial without constraints were mixed together, an exception was generated when drawing graphs.

Specifically, the null check was added because a Trial without constraints would be null.
In the case of null, Trial is feasible.

Copy link
Collaborator

@porink0424 porink0424 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 the PR!

Currently, type Trial.constraints is implemented in a way that null cannot be included (also, in the serialize_frozen_trial function of optuna_dashboard/_serializer.py, constraints are created like "constraints": trial_system_attrs.get(CONSTRAINTS_KEY, []), which means I don't think null will be included). So I'm not sure if it's really necessary to consider the possibility of null here.

If it is indeed necessary, could you provide an example code that produces such a situation?

@hrntsm
Copy link
Collaborator Author

hrntsm commented Sep 27, 2024

@porink0424

Thank you for checking it out.
I was not able to reproduce the problem, so I reviewed the log file and found that it was caused by the following null included in the file.

{"op_code": 9, "worker_id": "5609884b-ac1d-4851-854b-c2baa6e293e4-5712", "trial_id": 5, "system_attr": {"constraints": null}}

However, looking at the following Optuna implementation, it seems that it would result in an error in case of null.
Therefore, this kind of result should not occur.
As I can't understand the cause, I think I will close this PR for now.

https://github.com/optuna/optuna/blob/8476b460fb4ad79e9051a3b7ac49629d0e389ac9/optuna/samplers/_base.py#L244

@hrntsm hrntsm closed this Sep 27, 2024
@hrntsm hrntsm deleted the add-constraints-null-check branch November 11, 2024 08:33
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.

2 participants