Skip to content

fix(optuna-sweeper): pass values=None when marking trial as FAIL, add NaN check - #3333

Open
AbdullahRasheed45 wants to merge 2 commits into
hydra-ecosystem:mainfrom
AbdullahRasheed45:fix/optuna-sweeper-nan-values-crash
Open

fix(optuna-sweeper): pass values=None when marking trial as FAIL, add NaN check#3333
AbdullahRasheed45 wants to merge 2 commits into
hydra-ecosystem:mainfrom
AbdullahRasheed45:fix/optuna-sweeper-nan-values-crash

Conversation

@AbdullahRasheed45

Copy link
Copy Markdown

Problem

Closes #2237

When a Hydra run returns NaN objective values (e.g. a diverged model), the Optuna sweeper crashes with a double exception:

  1. study.tell(state=COMPLETE, values=[nan]) raises ValueError: Trial N failed, because the objective function returned nan.
  2. The except Exception handler sets state = TrialState.FAIL but still passes the original values to a second study.tell call, which raises another ValueError: Values cannot be specified when state is TrialState.PRUNED or TrialState.FAIL.

This second exception is unhandled and crashes the entire sweep.

Fix

Two-part change in plugins/hydra_optuna_sweeper/hydra_plugins/hydra_optuna_sweeper/_impl.py:

1. Proactive NaN check — after computing values, raise a descriptive ValueError if any value is NaN, so the failure is captured cleanly by the existing except block with a readable warning message.

2. Fix the except block — change study.tell(trial=trial, state=state, values=values) to study.tell(trial=trial, state=state, values=None). Optuna's API requires values=None when state is FAIL or PRUNED; passing the NaN values caused the secondary crash.

Behaviour after fix

A trial that returns NaN objective values is recorded as FAIL in the Optuna study (consistent with a diverged run), a warning is logged, and the sweep continues to the next trial instead of crashing.

… NaN check

When study.tell(state=COMPLETE, values=[nan]) raises a ValueError (optuna
rejects NaN objective values), the exception handler sets state=FAIL but then
calls study.tell(state=FAIL, values=[nan]) again — which raises a second
ValueError because optuna does not allow values to be specified for FAIL/PRUNED
trials.

Two-part fix:
1. Before calling study.tell, explicitly check whether any value in 'values' is
   NaN and raise a descriptive ValueError so the failure is captured by the
   existing except block with a helpful message.
2. In the except block, pass values=None to study.tell so the FAIL state is
   recorded without crashing with a secondary 'Values cannot be specified when
   state is PRUNED or FAIL' error.

Fixes hydra-ecosystem#2237
@meta-cla

meta-cla Bot commented Jul 31, 2026

Copy link
Copy Markdown

Hi @AbdullahRasheed45!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@omry

omry commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks.

  1. Please sign CLA, I cannot accept contributions without it.
  2. Please add a news fragment in the plugin.

@omry omry added the awaiting_response Awaiting response label Aug 1, 2026
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 1, 2026

@omry omry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks. The CLA and plugin news fragment are now in place, and changing the failed-trial retry to values=None matches the agreed fix in #2237 (comment).

Before this can be merged, please:

  1. Rebase the branch onto the latest main.
  2. Add a regression test reproducing the reported NaN objective case.
  3. Verify that the trial is recorded as FAIL, the sweep does not crash, and subsequent trials can continue.
  4. Run the Optuna sweeper test suite after rebasing.

The proactive math.isnan() check duplicates validation already performed by Optuna, but it also provides a clearer Hydra-controlled error message, so it can remain if it is covered by the regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting_response Awaiting response CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Optuna sweeper raises uncaught exception when run ends with nan value

2 participants