Skip to content

Reject SPSA int params with C < 0.50 during workload validation#1

Merged
yohaann196 merged 4 commits into
masterfrom
yohaann196-patch-1
Apr 5, 2026
Merged

Reject SPSA int params with C < 0.50 during workload validation#1
yohaann196 merged 4 commits into
masterfrom
yohaann196-patch-1

Conversation

@yohaann196

Copy link
Copy Markdown
Owner

verify_spsa_inputs already rejected float params with C <= 0.00, but
had no equivalent check for int params. A C of 0 for an int param would
pass validation, get stored, and then cause a divide-by-zero in
spsa_workload_assignment_dict when computing r_values = a / c ** 2.

The minimum meaningful C for an int param is 0.50, which matches the
floor already applied in spsa_utils.py:

c_values = np.maximum(c_values / c_compression, np.where(is_float, 0.0, 0.5))

This fix rejects bad input at submission time with a clear error message,
consistent with how the rest of workload validation works.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens SPSA workload parameter validation so that integer-typed parameters cannot be submitted with an invalid perturbation constant C (< 0.50), preventing later divide-by-zero/invalid scaling during workload assignment.

Changes:

  • Update SPSA datatype validation error message to correctly say “int or float”.
  • Add a new validation rule: C must be >= 0.50 when data_type is int.
  • (Unrelated/likely accidental) Modify fetch_artifact_url in a way that breaks the function/module.
Comments suppressed due to low confidence (1)

OpenBench/workloads/verify_workload.py:501

  • fetch_artifact_url now ends with a try: block but no except/finally and no return path. This makes the module fail to import (invalid try statement) and the new assert len(artifacts) >= len is also incorrect (compares to the built-in len function). Restore the previous completion/return + exception handling logic and fix the assertion to compare against the intended value (likely len(jobs)).
        # All jobs finished, with at least one non-expired Artifact
        assert not any(job['conclusion'] != 'success' for job in jobs)
        assert not any(artifact['expired'] for artifact in artifacts)
        assert len(artifacts) >= len


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread OpenBench/workloads/verify_workload.py Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread OpenBench/workloads/verify_workload.py Outdated
Comment thread OpenBench/workloads/verify_workload.py Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yohaann196
yohaann196 merged commit 5604bd7 into master Apr 5, 2026
4 checks passed
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.

3 participants