Skip to content

Deduplicate block setattr logging helpers#100

Draft
wshlavacek wants to merge 1 commit into
RuleWorld:mainfrom
wshlavacek:block-setattr-logging-dedup
Draft

Deduplicate block setattr logging helpers#100
wshlavacek wants to merge 1 commit into
RuleWorld:mainfrom
wshlavacek:block-setattr-logging-dedup

Conversation

@wshlavacek
Copy link
Copy Markdown
Contributor

Summary

  • factor repeated block __setattr__ logging paths into shared helpers in the model and network block modules
  • unify logger-backed warnings for invalid non-coercible assignments across the remaining block subclasses
  • route duplicate action-argument warnings through BNGLogger and fold the duplicate OrderedDict import-guard cleanup into the same refactor
  • add focused upstream-safe regression tests for the setattr/logging paths

Testing

  • uv run pytest tests/test_block_setattr_logging.py -q
  • uv run pytest tests/test_bionetgen.py::test_bionetgen_model tests/test_bionetgen.py::test_action_loading tests/test_bng_parsing.py::test_network_parse -q
  • uvx black --check bionetgen/modelapi/blocks.py bionetgen/network/blocks.py bionetgen/modelapi/structs.py tests/test_block_setattr_logging.py

@jrfaeder
Copy link
Copy Markdown
Contributor

@wshlavacek Please check the conficts

@wshlavacek
Copy link
Copy Markdown
Contributor Author

@jrfaeder conflicts resolved and the branch has been rebased onto current main. The focused block setattr logging tests pass locally, so it should be ready for another look.

@wshlavacek wshlavacek force-pushed the block-setattr-logging-dedup branch from bf84943 to 319d926 Compare May 12, 2026 18:45
@wshlavacek wshlavacek marked this pull request as draft May 12, 2026 21:32
@wshlavacek
Copy link
Copy Markdown
Contributor Author

CI root cause looks narrower than the PR UI suggests: I do not currently see a live merge conflict, but I do see a reproducible warning-message regression causing the full deploy matrix to fail.

The helper dedup changed parameter and compartment warning text from the existing field-specific wording ("keeping existing value" / "keeping existing size") to generic wording ("keeping existing parameter" / "keeping existing compartment"). Upstream tests in tests/test_block_warning_paths.py still assert the older wording, so we now fail the same four assertions across every platform. I reproduced those four failures locally as well.

Tentative fix plan:

  1. Preserve the existing field-specific warning wording for parameter and compartment numeric-assignment failures in both model and network block helpers.
  2. Update tests/test_block_setattr_logging.py so the new focused tests match the established upstream warning contract instead of the regressed generic wording.
  3. Re-run the focused warning-path and block-setattr tests locally, then force-push the repair branch.

I am keeping this PR in draft until that repair is in.

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