Re-vendor RuleMonkey 3.9.0 to 3.10.0 - #480
Merged
Merged
Conversation
The vendored copy was eleven commits behind. Three of those change what a model does under method="rm", and one can turn a model that runs today into one that refuses, so this belongs in a release that is already a major bump. TotalRate now warns at load and is refused for one shape. BioNetGen does not implement TotalRate for network simulations, so there is no BNG2 result to check such a model against, and NFsim disagrees with RuleMonkey on it. NFsim expands a rule whose reactant pattern has interchangeable components into one reaction class per permutation, and under TotalRate every class returns the whole rate, so the rule runs at the rate times the number of populated permutations. Upstream measured 1.00x, 2.02x and 2.98x as free sites go from one to three, and BNG2 implies a third number again. All three disagree, so RuleMonkey refuses those rules rather than picking a reading silently. The test is narrow on purpose. A TotalRate rule is refused only when a reactant pattern touches a component whose molecule type declares two or more of that name. Everything else warns and runs. A rate law built on reactant_N() had a propensity of zero, so any rule using one never fired. It is now resolved against the rule asking for it. A session counted every tracked observable twice. The rest is either a fix to a shape our corpus does not write or performance work with no behavioural component. This refresh could not run until richardposner/RuleMonkey#78 merged. RuleMonkey keeps its own copy of the bngsim expression evaluator for standalone builds, and the EXPRTK_SYNC_FILES guard compares that copy against this tree and fails closed. It was pinned to a bngsim commit predating the mratio work of issues #451, #453 and #456. That upstream refresh changes nothing here, because inside a bngsim build CMake links the host bngsim::expression target and the vendored copy is never compiled. That is also why the drift went unnoticed. The ordering is now recorded in scripts/RULEMONKEY_VENDORING.md, whose baseline was stale as well: it named commit 13e9f636 while VENDOR.json said 1d14160. VENDOR.json had recorded a previous session's scratch directory as local_checkout. This refresh ran from /tmp/rulemonkey-vendor-candidate, the path the vendoring doc prescribes, so that record is a stable path now. Full Python suite: 5612 passed, 24 skipped.
scripts/local_ci.py writes scripts/_reports/ and a scripts/local_ci_report-<platform>-<pytag>.md next to itself. Neither has ever been committed and neither should be, since both are machine specific. They were not ignored either, so running the wheel smoke that RULEMONKEY_VENDORING.md asks for leaves two untracked paths sitting in git status, one of them a directory. Found by running that step for this pull request.
Collaborator
Author
|
Wheel smoke result, the packaging check RULEMONKEY_VENDORING.md asks for: RuleMonkey session PASS is the line that matters here, since it exercises the I also added a second commit while running that step. scripts/local_ci.py writes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refreshes the vendored RuleMonkey from 3.9.0 to 3.10.0, for 0.15.0.
Why now
The vendored copy was eleven commits behind. Three of those change what a model
does under method="rm", and one of them can turn a model that runs today into one
that refuses, so this belongs in a release that is already a major bump rather
than in a patch.
What changes for users
TotalRate now warns at load, and is refused for one specific shape. BioNetGen
does not implement TotalRate for network simulations, so there is no BNG2 result
to check such a model against, and NFsim disagrees with RuleMonkey on it. NFsim
expands a rule whose reactant pattern has interchangeable components into one
reaction class per permutation, and under TotalRate every class returns the whole
rate, so the rule runs at the rate times the number of populated permutations.
Upstream measured 1.00x, 2.02x and 2.98x as free sites go from one to three. BNG2
implies a third number again. All three disagree, so RuleMonkey refuses those
rules rather than picking one reading silently. The test is narrow on purpose: a
TotalRate rule is refused only when a reactant pattern touches a component whose
molecule type declares two or more of that name. Everything else warns and runs.
A rate law built on reactant_N() had a propensity of zero, so any rule using one
never fired at all. It is now resolved against the rule asking for it.
A session counted every tracked observable twice.
The rest is either a fix to a shape our corpus does not write, or performance
work with no behavioural component.
The two-repo part, which is the awkward bit
This refresh could not run at all until richardposner/RuleMonkey#78 merged.
RuleMonkey keeps its own copy of bngsim's expression evaluator for standalone
builds, and the EXPRTK_SYNC_FILES guard compares that copy against this tree and
fails closed. The copy was pinned to a bngsim commit predating the mratio work of
issues #451, #453 and #456, so the guard refused with:
That upstream refresh changes nothing here. Inside a bngsim build CMake links the
host bngsim::expression target and the vendored copy is never compiled, which is
exactly why the drift went unnoticed for so long. It did mean standalone
RuleMonkey was still evaluating mratio with the version that can be silently
wrong, which is the reason it was worth landing upstream.
I added a note recording this ordering to scripts/RULEMONKEY_VENDORING.md, whose
Current Baseline was also stale. It named commit 13e9f636 while VENDOR.json said
1d14160.
One small cleanup
VENDOR.json recorded a previous session's scratch directory as local_checkout.
This refresh was run from /tmp/rulemonkey-vendor-candidate, the path the vendoring
doc prescribes, so that record is now a stable path rather than a machine-specific
one.
Testing
Full Python suite against the new tree, and the targeted suites the vendoring doc
names:
5612 passed, 24 skipped, nothing failed. The count is 9 higher than before
because #478 landed its own tests on main in the meantime.
63 passed. The changelog and vendoring lint suites were run separately after the
CHANGELOG.md and RULEMONKEY_VENDORING.md edits, since those came after the full
run: 35 passed.
The wheel smoke that the vendoring doc names as the packaging truth for
RuleMonkeySession is running and I will add its result as a comment.
Verified separately that the vendored tree carries no bngsim_expr directory and
that its CMakeLists still takes the if(TARGET bngsim::expression) handoff, which
is the guardrail against duplicate ExprTkEvaluator symbols.
Not included
RuleWorld/nfsim#92 is still open, so the NFsim carry queue is untouched and #428
stays open as its tracker.