Skip to content

Replace flax usage with equinox - #1246

Open
jd61349 wants to merge 22 commits into
mainfrom
refactor/flax-to-equinox
Open

jd61349 wants to merge 22 commits into
mainfrom
refactor/flax-to-equinox

Conversation

@jd61349

@jd61349 jd61349 commented Sep 11, 2026

Copy link
Copy Markdown
Member

PR Type

  • Refactoring (no functional changes)

Description

Replace all usage of Flax with Equinox. Fixes #582.

How Has This Been Tested?

Related tests have been changed to reflect updated code, however what they test has remained the same. All tests pass.

Does this PR introduce a breaking change?

No.

Checklist before requesting a review

  • I have made sure that my PR is not a duplicate.
  • My code follows the style guidelines of this project.
  • I have ensured my code is easy to understand, including docstrings and comments where necessary.
  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.
  • I have updated CHANGELOG.md, if appropriate.

@CLAassistant

CLAassistant commented Sep 11, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@jd61349
jd61349 force-pushed the refactor/flax-to-equinox branch from 33612ee to de5ac11 Compare September 11, 2026 08:55
@jd61349
jd61349 marked this pull request as ready for review September 11, 2026 09:15
@jd61349 jd61349 changed the title Refactor/flax to equinox Replace flax usage with equinox Sep 11, 2026

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MLP.__init__ now passes mode="batch" to eqx.nn.BatchNorm, but pyproject.toml still supports equinox>=0.11.11. In Equinox 0.11.11 BatchNorm.__init__ has no mode argument, so a lowest-resolution install fails as soon as this benchmark constructs the model. Could we either raise the lower bound to the version that introduced this API or avoid the newer argument, and add a lowest-dependency benchmark smoke test?

@jd61349

jd61349 commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

MLP.__init__ now passes mode="batch" to eqx.nn.BatchNorm, but pyproject.toml still supports equinox>=0.11.11. In Equinox 0.11.11 BatchNorm.__init__ has no mode argument, so a lowest-resolution install fails as soon as this benchmark constructs the model. Could we either raise the lower bound to the version that introduced this API or avoid the newer argument, and add a lowest-dependency benchmark smoke test?

Good spot! Thanks

@jd61349
jd61349 force-pushed the refactor/flax-to-equinox branch from f23b47f to 15a48da Compare September 14, 2026 13:03

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Performance review

Commit d6460a3 - Merge 45e5a4b into 9a804d3

Statistically significant changes

  • basic_rpc:
    • OLD: compilation 0.7062 units ± 0.01754 units; execution 0.02056 units ± 0.0001399 units
    • NEW: compilation 0.6986 units ± 0.03884 units; execution 0.02276 units ± 0.0002806 units
    • Significant increase in execution time (10.70%, p=7.695e-12)

Normalisation values for new data:
Compilation: 1 unit = 646.35 ms
Execution: 1 unit = 697.58 ms

Last updated: Wed, 16 Sep 2026 11:46:16 GMT

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed current 15a48daf. The compatibility issue I raised is resolved: the Equinox lower bound is now >=0.13.0, matching the BatchNorm(..., mode="batch") API used by the benchmark, and the lowest/highest dependency test matrix is green across supported Python versions. No remaining blocker from me.

@tm91236 tm91236 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, but it does introduce some breaking changes without any deprecation warnings. I think we should keep the old network.py module, but mark the ScoreNetwork as deprecated for removal in v1.2.0.

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml
Comment thread .pylintrc

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed current 45e5a4b5. Restoring coreax/networks.py for the requested deprecation path introduces a new compatibility problem: that module imports flax, but this PR still removes flax from the package dependencies. A normal installation can therefore no longer import the deprecated module that is being kept specifically to preserve compatibility. The current pre-commit run confirms this with unresolved imports for flax, flax.linen, and flax.training in coreax/networks.py. Please retain the flax dependency for the deprecation window (or otherwise make the compatibility module self-contained) and add a simple normal-install import regression for coreax.networks/ScoreNetwork.

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.

Replace flax usage with equinox

4 participants