-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support human-in-the-loop via trial.user_attrs
#411
Conversation
Let me work on refactoring (removing "objective" from the internal variable names, etc.) by follow-up PR. |
Co-authored-by: Masashi Shibata <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knshnb Thank you for the update! Looks almost good to me. I left one suggestion though.
ObjectiveSliderWidget = SliderWidget | ||
ObjectiveTextInputWidget = TextInputWidget | ||
FORM_WIDGETS_KEY = "dashboard:form_widgets:v2" | ||
FORM_WIDGETS_OUTPUT_TYPE_KEY = "dashboard:form_widgets_output_type:v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if FORM_WIDGETS_OUTPUT_TYPE_KEY
could be included in FORM_WIDGETS_KEY
as follows.
FormWidgetJSON = TypedDict( "FormWidgetJSON", { "output_type": Literal["objective", "user_attr"], "widgets": list[Union[ChoiceWidgetJSON, SliderWidgetJSON, TextInputWidgetJSON, UserAttrRefJSON]] } ) form_widgets: FormWidgetJSON = { "output_type": "user_attr", "widgets": [w.to_dict() for w in widgets], } study._storage.set_study_system_attr(study._study_id, FORM_WIDGETS_KEY, form_widgets)
Please see d0cb33e for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I refactored form_widgets_output_type according to your comment.
@c-bata Thanks for the review! I addressed your comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knshnb Thanks for the update!
I realized that the values in ReadonlyObjectiveForm for register_user_attr_form_widgets are not displayed correctly, but would it be fine to discuss how to fix it as a follow-up PR?
Sure! I think it's acceptable if the problem does not affect users who are not using register_user_attr_form_widgets()
.
Changes looks almost good to me. Though I have some minor additional review comments, I will address them on #431.
(See d6f3846 for details).
Follow-up #411 to refactor type definitions
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.
Reference Issues/PRs
register_objective_form_widgets
was supported by #370. This PR addsregister_user_attr_form_widgets
, which registers the human evaluations totrial.user_attrs
. With this, we can specify an objective function that combines multiple human evaluations (and suggested parameters).What does this implement/fix? Explain your changes.
Example: