Skip to content

Handle configs via dependency injection#931

Draft
cpaniaguam wants to merge 44 commits intorlssm-class-make-model-distfrom
930-pass-configs-via-dependency-injection-into-model-classes-basemodelconfig-only-dict-supported
Draft

Handle configs via dependency injection#931
cpaniaguam wants to merge 44 commits intorlssm-class-make-model-distfrom
930-pass-configs-via-dependency-injection-into-model-classes-basemodelconfig-only-dict-supported

Conversation

@cpaniaguam
Copy link
Copy Markdown
Collaborator

@cpaniaguam cpaniaguam commented Mar 11, 2026

This pull request refactors the model configuration and initialization logic for the HSSM base class, moving the responsibility for constructing and validating model configurations out of HSSMBase and into the configuration classes themselves. This results in a cleaner, more modular design and simplifies subclassing and serialization. The most important changes are grouped below.

Refactoring and Simplification of Model Configuration

  • The HSSMBase class now requires a fully initialized BaseModelConfig (usually a Config) to be passed in, rather than accepting a mix of model names, config dicts, and other parameters. All likelihood, parameter, and data information is now drawn from this object, and the constructor no longer builds the config itself. [1] [2] [3]
  • The logic for building and validating a model config from defaults, user input, and external sources (like ssms-simulators) has been moved to a new _build_model_config classmethod on Config. This method is now responsible for all config normalization and validation.
  • The _build_model_config and get_config_class methods have been removed from HSSMBase and BaseModelConfig, and the responsibility for config class resolution is now handled by the config classes themselves. [1] [2] [3] [4]

Improved Initialization Argument Capture and Serialization

  • The mechanism for capturing constructor arguments for save/load serialization has been improved. A new static method _store_init_args provides a robust way for subclasses to snapshot their initialization arguments, and a default empty snapshot is set in the base class to prevent serialization errors if subclasses forget to call it. [1] [2] [3]
  • The __getstate__ method now raises a clear error if the initialization snapshot is missing, making the contract for serialization explicit and robust.

API and Documentation Updates

  • The constructor signature and docstrings for HSSMBase have been updated to reflect the new configuration workflow, clarifying that a BaseModelConfig is required and describing its role. [1] [2]
  • Convenience properties for the number of parameters and extra fields have been added to BaseModelConfig and cleaned up in RLSSMConfig. [1] [2]

Internal Imports and Logging

  • Imports related to model configuration have been cleaned up and moved to the appropriate files. Logging for model configuration choices has been centralized in config.py. [1] [2] [3]

@cpaniaguam cpaniaguam changed the base branch from main to rlssm-class-make-model-dist March 11, 2026 18:08
@cpaniaguam cpaniaguam requested a review from Copilot March 11, 2026 18:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 refactors HSSM/RLSSM configuration handling to use typed config objects built via factory methods (dependency injection into HSSMBase), and updates RLSSM internals/tests to align with the new config and attribute patterns.

Changes:

  • Refactors HSSMBase to accept a pre-built BaseModelConfig and adds deprecated proxy properties for legacy attributes.
  • Adds Config._build_model_config(...) and uses it in HSSM.__init__; RLSSM now builds a validated Config via RLSSMConfig._build_model_config(...).
  • Updates RLSSM public attributes/tests (n_participants, n_trials) and adjusts distribution construction to work with the new config flow.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_rlssm.py Updates tests to use new RLSSM public attributes.
src/hssm/rl/rlssm.py Removes config mixin inheritance; injects validated config into HSSMBase.
src/hssm/hssm.py Adds HSSM.__init__ that builds a validated Config via factory then calls HSSMBase.
src/hssm/config.py Introduces centralized config factory (Config._build_model_config) and RLSSM helper factory.
src/hssm/base.py Changes base initializer to consume injected config and adds deprecated proxy properties + init-arg capture helper.

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

You can also share your feedback on Copilot code review. Take the survey.

src/hssm/base.py Outdated
Comment on lines +387 to +391
warnings.warn(
f"`{name}` is deprecated; use `self.model_config.{name}` instead.",
DeprecationWarning,
stacklevel=2,
)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The new deprecated proxy properties always emit DeprecationWarning via _deprecation_warn(). Since core internals (including HSSMBase.__init__) call self.list_params, self.choices, etc., this will trigger warnings during normal model construction even when users never touch deprecated attributes (and can break in environments that treat warnings as errors). Consider having internal code access self.model_config.* directly, or gating warnings so they only fire for external/user-level access.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 5 out of 5 changed files in this pull request and generated 4 comments.


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

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 6 out of 6 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/hssm/base.py:276

  • After introducing deprecated proxy properties (e.g., self.list_params, self.choices), this initializer still uses them internally (_validate_choices(), if self.list_params is None, len(self.choices)). That will emit DeprecationWarnings during normal model construction (especially under warnings-as-errors). Prefer direct access to the authoritative model_config here (e.g., model_config.list_params, model_config.choices).
        self._validate_choices()

        # region Avoid mypy error later (None.append). Should list_params be Optional?
        if self.list_params is None:
            raise ValueError(

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

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 8 out of 8 changed files in this pull request and generated 1 comment.


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

You can also share your feedback on Copilot code review. Take the survey.

…cify required fields and improve readability.
… names in parameter mapping for safe unpickling.
…BaseModelConfig instance and clarify usage of dict for configuration.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/hssm/config.py:227

  • update_choices assigns choices directly, but callers (e.g. Config._build_model_config) can pass a list[int]. This breaks the tuple-based immutability/type-consistency the rest of the module is moving toward and can leak mutable lists into Config.choices. Coerce to tuple(choices) (and consider widening the parameter type to accept Sequence[int]).
    def update_choices(self, choices: tuple[int, ...] | None) -> None:
        """Update the choices from user input.

        Parameters
        ----------
        choices : tuple[int, ...] | None
            A tuple of choices.
        """
        if choices is None:
            return

        self.choices = choices


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

You can also share your feedback on Copilot code review. Take the survey.

@cpaniaguam cpaniaguam requested a review from digicosmos86 March 13, 2026 16:44
@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cpaniaguam cpaniaguam changed the title 930 pass configs via dependency injection into model classes basemodelconfig only dict supported Handle configs via dependency injection Mar 17, 2026
@cpaniaguam cpaniaguam self-assigned this Mar 17, 2026
Copy link
Copy Markdown
Collaborator

@digicosmos86 digicosmos86 left a comment

Choose a reason for hiding this comment

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

Thanks @cpaniaguam! Adding a few comments. Let me know what you think

Copy link
Copy Markdown
Member

@AlexanderFengler AlexanderFengler left a comment

Choose a reason for hiding this comment

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

ok to go for me.

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.

Pass configs via dependency injection into model classes (BaseModelConfig-only; dict supported)

4 participants