feat: Add registry for reservation rule classes by tvk134 · Pull Request #357 · sequence-toolbox/SeQUeNCe

tvk134 · 2026-05-23T22:11:24Z

Summary:

Refactor reservation-generated resource-management rules into individual Rule subclasses.
Add a ReservationRuleRegistry that maps reservation rule names to Rule constructors.
Add a DefaultReservationRuleGenerator that uses the registry to construct rules for a reservation.
Update ResourceManager.generate_load_rules to delegate rule construction while preserving rule scheduling, expiration, and memory-reset behavior.
Expose ResourceManager.get_reservation_rule_registry() so users can disable or replace individual reservation rules without subclassing the generator.

Testing:

uv run ruff check sequence/resource_management/resource_manager.py sequence/resource_management/reservation_rule_registry.py tests/resource_management/test_reservation_rule_registry.py
uv run pytest tests/resource_management -vv
uv run pytest tests/network_management -vv
uv run pytest tests/topology -vv
uv run pytest tests/app -vv
uv run pytest tests/components -vv
uv run pytest tests/entanglement_management -vv
uv run pytest tests/kernel -vv
uv run pytest tests/qkd -vv
uv run pytest tests/qlan -vv
uv run pytest tests/utils -vv

Note:

Full uv run pytest -vv hangs locally during collection on Windows at 228 collected items, including after a restart and several minutes of waiting.
Running each top-level test directory individually completes successfully.

tvk134 · 2026-06-03T01:57:25Z

Pushed the discussed updates.

Summary:

Renamed DefaultReservationRuleGenerator to ReservationRuleGenerator.
Removed the custom registry injection path; the generator now owns its registry initialized with the default reservation rule constructors.
Kept ResourceManager.get_reservation_rule_registry() as the configuration hook for replacing/disabling individual reservation-generated rules.
Made reservation rule priority an init parameter with the existing default value of 10.
Added a generator-level test verifying that a registered replacement rule is actually produced by create_rules(...), not just stored in the registry.
Cleaned up the reservation rule registry tests.

Validation:

uv run pytest tests/resource_management/test_reservation_rule_registry.py
- All 4 passed
uv run pytest tests/resource_management
- All 16 passed

hayekr · 2026-06-20T01:02:59Z

Please resolve your conflict.

Tw1ZZLER

This PR mostly reads like a prompt-driven refactor, and not an intentional redesign of how a good reservation rule registry should work. I would seriously spend more time thinking about how the API should be re-engineered for this.

I do think the addition of _get_swapping_neighbors was a great change! The introduction of this method makes the readability much much better and gives the loop a good name.

However, extensibility is pretty overstated, and in practice most users would still have to override a lot of the methods and existing framework. Please see comments for other thoughts.

I really think the motivation behind this change is a good one, but the execution does not feel very thought out in terms of engineering and design.

Tw1ZZLER · 2026-07-07T21:32:53Z

The PR's scope is not clear given the description of the PR and the code itself. It appears that you are trying to partially implement a completely generalized solution for a rule registry, that allows users to add rules and change the order of rules however they see fit.

If you are going for a simpler system that only replaces existing rules with custom args, then I would suggest using a simple table of (name, predicate, builder) instead of classes for each rules, and make it explicit that the registry only allows the user to replace the reservation rules with custom ones.

I think there was a discrepancy in the description and the code itself versus the purpose and scope. Overall I think the implementation would make a good contribution towards SeQUeNCe, but it is a little over-engineered for the simple "replace rules easily" scope. Try and make it simpler and narrow the scope and it will be a very good addition I think 🙂

Ignore the comments from my review until you narrow the scope. I will resolve comments that fall outside of the new scope or are outdated 💯

tvk134 · 2026-07-11T14:26:18Z

Thanks @Tw1ZZLER , this clarification helped. I revised the PR around the narrower intended scope: this is now explicitly a fixed-scope replacement mechanism for SeQUeNCe’s existing reservation-generated rule slots, not a generalized registry for adding arbitrary reservation phases or changing scheduling order.

The implementation now uses a static table of (name, predicate, builder) specs instead of separate Rule subclasses. The registry allows replacing or disabling builders for those existing rule slots, while the rule creation order and predicates remain fixed. I also preserved the ES swapping success/degradation action arguments, removed the dead/misleading API pieces from the earlier version, and updated the tests for the narrowed API.

I ran the pytest suite locally and all tests pass. Please let me know if I missed anything from the intended scope or if there are any additional changes you’d like me to make.

Tw1ZZLER

Looks great!! You did an amazing job implementing my idea, probably much better than I would have done so. create_rules() is now such a clean method it makes me happy just looking at it! Definitely a massive improvement over what it used to be before this PR, and gives the user more customizability for sure. APPROVE!

I added 2 comments for adding additional documentation in some spots that would be nice, but not blocking the merge.

Tw1ZZLER · 2026-07-17T20:25:42Z

        pass

+    def get_reservation_rule_registry(self) -> ReservationRuleRegistry:
+        """Return the registry used to replace or disable reservation rule builders."""


Can you add a usage example here showing how to customize a rule? It will help for documentation

Tw1ZZLER · 2026-07-17T20:34:31Z

+
+
+def _get_swapping_neighbors(owner_name: str, path: list[str]) -> tuple[str, str]:
+    """Return the left and right neighbors used for middle-node swapping rules."""


Can you explain the algorithm used here just a bit more?

caitaozhan · 2026-07-17T22:40:26Z

+        "path": context.path,
+        "index": context.index,
+    }
+    return Rule(10, eg_rule_action_await, eg_rule_condition, action_args, condition_args)


Priority cannot be fixed at 10

caitaozhan · 2026-07-17T22:43:34Z

+    else:
+        condition_args = {"memory_indices": context.memory_indices[context.reservation.memory_size:]}
+
+    action_args = {


What if I want to change the action_args?

See an example of customizing the action_args by adding "memo_type" https://github.com/haydenmllr1317/heterogenous/blob/be30fa92a4e101a6ac64609d4232889d0fc430ca/resource_manager.py#L164

tvk134 requested review from Alex-Kolar, JoaquinChung, caitaozhan and hayekr as code owners May 23, 2026 22:11

caitaozhan reviewed May 29, 2026

View reviewed changes

Comment thread sequence/resource_management/reservation_rule_registry.py Outdated

hayekr reviewed May 29, 2026

View reviewed changes

Comment thread sequence/resource_management/reservation_rule_registry.py Outdated

hayekr changed the title ~~Add registry for reservation rule classes~~ feat: Add registry for reservation rule classes Jun 30, 2026

hayekr requested a review from Tw1ZZLER July 6, 2026 03:07

Tw1ZZLER requested changes Jul 6, 2026

View reviewed changes

tvk134 added 3 commits July 11, 2026 21:54

Add registry for reservation rule classes

8a20fc6

Refine reservation rule registry API

15d0538

Simplify reservation rule registry scope

ef10699

tvk134 force-pushed the rule-builder-registry branch from 0073ed1 to ef10699 Compare July 11, 2026 14:22

github-actions Bot added the feat label Jul 11, 2026

tvk134 requested a review from Tw1ZZLER July 13, 2026 17:03

Tw1ZZLER approved these changes Jul 17, 2026

View reviewed changes

caitaozhan reviewed Jul 17, 2026

View reviewed changes

caitaozhan requested changes Jul 17, 2026

View reviewed changes



		def _get_swapping_neighbors(owner_name: str, path: list[str]) -> tuple[str, str]:
		"""Return the left and right neighbors used for middle-node swapping rules."""

Uh oh!

Conversation

tvk134 commented May 23, 2026

Uh oh!

Uh oh!

Uh oh!

tvk134 commented Jun 3, 2026

Uh oh!

hayekr commented Jun 20, 2026

Uh oh!

Tw1ZZLER left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tw1ZZLER commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvk134 commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tw1ZZLER left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tw1ZZLER Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Tw1ZZLER Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

caitaozhan Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

caitaozhan Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tw1ZZLER commented Jul 7, 2026 •

edited

Loading

tvk134 commented Jul 11, 2026 •

edited

Loading

Tw1ZZLER left a comment •

edited

Loading

caitaozhan Jul 17, 2026 •

edited

Loading