Skip to content

refactor(edm): invert ownership between AF3EDMSampler and EDMSamplerConfig#170

Merged
marcuscollins merged 3 commits intomainfrom
dm/issue-79
Mar 15, 2026
Merged

refactor(edm): invert ownership between AF3EDMSampler and EDMSamplerConfig#170
marcuscollins merged 3 commits intomainfrom
dm/issue-79

Conversation

@DorisMai
Copy link
Copy Markdown
Contributor

@DorisMai DorisMai commented Mar 14, 2026

Address issue #79

Changes that might benefit from extra attention is in conftest.py, where I made changes to class ComponentInfo, SAMPLER_REGISTRY and create_sampler_from_type to handle this refactor.

Summary by CodeRabbit

  • Refactor
    • Sampler initialization consolidated to use a single EDMSamplerConfig object instead of multiple separate parameters.
    • Sampler internals updated to read settings from the config object.
  • Tests
    • Tests and fixtures updated to construct and pass the new config object when creating samplers.
  • Public API
    • Sampler constructor now accepts a single config parameter.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 30396264-a121-4fc1-9484-bb59ad9f384a

📥 Commits

Reviewing files that changed from the base of the PR and between 81638d7 and 1d91b13.

📒 Files selected for processing (2)
  • src/sampleworks/core/samplers/edm.py
  • tests/conftest.py

📝 Walkthrough

Walkthrough

Refactors AF3EDMSampler to be config-driven: EDMSamplerConfig is introduced/used as the single initializer argument for AF3EDMSampler, the EDMSamplerConfig.create_sampler() factory was removed, and all internal sampler accesses were migrated to use self.config.*. Call sites, utilities, fixtures, and tests updated accordingly.

Changes

Cohort / File(s) Summary
Core sampler refactoring
src/sampleworks/core/samplers/edm.py
Removed EDMSamplerConfig.create_sampler(). Converted AF3EDMSampler to accept a single config: EDMSamplerConfig via __init__. Replaced direct field references with self.config.* and updated methods to read config values.
Utilities
src/sampleworks/utils/guidance_script_utils.py
Import EDMSamplerConfig and construct a config object; instantiate AF3EDMSampler(config=...) instead of passing multiple keyword args.
Test fixtures & registry
tests/conftest.py
Added EDMSamplerConfig import. Added config_class_path to ComponentInfo and updated SAMPLER_REGISTRY entry for AF3EDM. create_sampler_from_type() now dynamically imports/constructs config when config_class_path is set. Updated edm_sampler fixture to instantiate sampler with a config object.
Integration tests
tests/integration/test_mismatch_integration.py, tests/integration/test_pipeline_integration.py
Updated imports and all AF3EDMSampler instantiations to pass EDMSamplerConfig(...) instead of multiple keyword arguments.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Poem

🐰 I nibbled code and stitched a seam,

One config to rule the sampler stream,
Fields condensed, the paths align,
I hop along — the changes shine! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main refactoring: inverting ownership so EDMSamplerConfig is passed into AF3EDMSampler rather than AF3EDMSampler being created from EDMSamplerConfig.
Docstring Coverage ✅ Passed Docstring coverage is 91.30% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dm/issue-79
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DorisMai DorisMai marked this pull request as ready for review March 14, 2026 21:17
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/sampleworks/core/samplers/edm.py (1)

156-157: Document the new config-driven constructor.

AF3EDMSampler now accepts a single EDMSamplerConfig, but __init__ has no NumPy-style docstring and the class docstring still reads like the sampler knobs are passed directly to the constructor. Please document config here or update the class docstring to match the new API.

As per coding guidelines, **/*.py: Always include NumPy-style docstrings for every function and class.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sampleworks/core/samplers/edm.py` around lines 156 - 157, The constructor
__init__(self, config: EDMSamplerConfig) lacks a NumPy-style docstring and the
AF3EDMSampler class docstring still implies individual sampler knobs are passed;
update either the AF3EDMSampler class docstring or add a NumPy-style docstring
to __init__ that clearly documents the single parameter config (type
EDMSamplerConfig), its fields/purpose, and any defaults/side-effects so the
public API matches the new config-driven initialization; reference the
AF3EDMSampler class and its __init__ method and ensure the new docstring follows
the project's NumPy-style docstring conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/conftest.py`:
- Around line 249-256: The code in the config_class_path branch constructs a
config from info.default_kwargs and then calls create_component_from_info while
create_component_from_info rebuilds kwargs from info.default_kwargs again, which
re-applies defaults and can override config-backed constructor defaults; fix by
passing the constructed config directly to create_component_from_info (use the
existing variable config and do not rebuild or pass
info.default_kwargs/extra_kwargs into create_component_from_info) so
instantiation uses the config object's values; locate the branch referencing
config_class_path, config_cls, config_kwargs, and the call to
create_component_from_info and remove the redundant reconstruction of
default_kwargs before instantiation.

---

Nitpick comments:
In `@src/sampleworks/core/samplers/edm.py`:
- Around line 156-157: The constructor __init__(self, config: EDMSamplerConfig)
lacks a NumPy-style docstring and the AF3EDMSampler class docstring still
implies individual sampler knobs are passed; update either the AF3EDMSampler
class docstring or add a NumPy-style docstring to __init__ that clearly
documents the single parameter config (type EDMSamplerConfig), its
fields/purpose, and any defaults/side-effects so the public API matches the new
config-driven initialization; reference the AF3EDMSampler class and its __init__
method and ensure the new docstring follows the project's NumPy-style docstring
conventions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fe38ca3b-5cf0-45b3-a42c-a461ee1f4254

📥 Commits

Reviewing files that changed from the base of the PR and between 76d922c and 81638d7.

📒 Files selected for processing (5)
  • src/sampleworks/core/samplers/edm.py
  • src/sampleworks/utils/guidance_script_utils.py
  • tests/conftest.py
  • tests/integration/test_mismatch_integration.py
  • tests/integration/test_pipeline_integration.py

Comment thread tests/conftest.py Outdated
@marcuscollins marcuscollins merged commit e5db928 into main Mar 15, 2026
3 of 4 checks passed
@k-chrispens k-chrispens deleted the dm/issue-79 branch April 22, 2026 00:26
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.

2 participants