Skip to content

feat(training): expose curriculum parameters (depth_slope and max_depth) at training time#139

Merged
cbjuan merged 8 commits intoQiskit:mainfrom
richardesp:feat/expose-curriculum-parameters
Mar 11, 2026
Merged

feat(training): expose curriculum parameters (depth_slope and max_depth) at training time#139
cbjuan merged 8 commits intoQiskit:mainfrom
richardesp:feat/expose-curriculum-parameters

Conversation

@richardesp
Copy link
Contributor

@richardesp richardesp commented Feb 16, 2026

Description

This PR extends start_training to support curriculum parameters configuration by exposing depth_slope and max_depth as optional parameters.

Linked Issue(s)

Fixes #130

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • test_start_training_applies_curriculum_overrides: Test that depth_slope and max_depth overrides are applied to the environment instance before RLSynthesis is instantiated.
  • test_start_training_invalid_depth_slope: Test error when depth_slope is less than 1.
  • test_start_training_invalid_max_depth: Test error when max_depth is less than 1.

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Move parameter validation into a dedicated helper function to
reduce branching.

This refactor resolves Ruff warning:

PLR0912 Too many branches (14 > 12)
--> qiskit-gym-mcp-server/src/qiskit_gym_mcp_server/training.py:248:11
@richardesp richardesp marked this pull request as ready for review February 25, 2026 17:28
@cbjuan
Copy link
Member

cbjuan commented Mar 6, 2026

Code review

Overall this is a clean, well-structured feature addition. Two items worth discussing:

1. Dead assertions in test_start_training_applies_curriculum_overrides

The config-dict assertions on lines 175-179 and 186-189 never execute. passed_env is a MagicMock, so getattr(passed_env, "config", None) returns another MagicMock (not a dict), making isinstance(cfg, dict) always False. This means the _set_in_config code path has zero test coverage.

assert passed_env.max_depth == 64
cfg = getattr(passed_env, "config", None)
if isinstance(cfg, dict):
assert cfg.get("depth_slope") == 3
assert cfg.get("max_depth") == 64

One fix: set up the mock with a real config dict before calling start_training:

mock_instance = mock_permutation_gym.return_value
mock_instance.config = {}
mock_instance._raw_env.config = {}

2. _set_if_present silently skips overrides when attributes don't exist

If the gym instance doesn't already have depth_slope/max_depth as pre-existing attributes, the override is silently dropped. The user gets no indication their parameters were ignored. A logger.debug() or logger.warning() when skipping would help with troubleshooting.

setattr(obj, name, value)
def _set_in_config(obj: Any, name: str, value: int) -> None:
cfg = getattr(obj, "config", None)

🤖 Generated with Claude Code

@richardesp
Copy link
Contributor Author

Thanks for the suggestions! The first point has been addressed in this commit: c857587, and the second one was fixed here: a3ead1a

Copy link
Member

@cbjuan cbjuan left a comment

Choose a reason for hiding this comment

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

Both review items addressed. Tests now properly cover all override paths (attributes, config dicts, _raw_env), and the warning log improves debuggability. LGTM.

@cbjuan
Copy link
Member

cbjuan commented Mar 11, 2026

The PR will be merged and included in a release after the 1st qiskit-docs-mcp-server gets published. So likely this will wait for #143 and #144

@cbjuan cbjuan merged commit 5c96777 into Qiskit:main Mar 11, 2026
26 of 30 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.

FEATURE: Expose curriculum parameters (depth_slope, max_depth) at training time via environment injection

2 participants