Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The shared loading path addresses the reported multiprocessing failures with comprehensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds consistent extension loading across CLI, Python API, and multiprocessing workers.
Changes:
- Normalizes extension locations and uses a shared loader.
- Propagates working-directory context to workers.
- Adds cross-platform regression tests and documentation.
| File | Description |
|---|---|
docs/dev-guide/core-workflow-api.md |
Documents extension loading semantics. |
activitysim/core/extensions.py |
Implements shared resolution and import helpers. |
activitysim/core/workflow/state.py |
Registers normalized extension locations. |
activitysim/core/workflow/runner.py |
Safely forwards available worker injectables. |
activitysim/core/mp_tasks.py |
Reloads extensions through the shared state API. |
activitysim/cli/run.py |
Delegates extension loading and preserves working directories. |
activitysim/core/test/test_extensions.py |
Tests loader and worker behavior. |
activitysim/cli/test/test_extensions.py |
Tests CLI and API execution end to end. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.
Problem and fix
activitysim run --ext model/extensionsimports the package in the parent but fails in multiprocessing because the worker passes the filesystem path toimportlib.import_module.State.import_extensions()records the same path and has the same worker failure. Absolute paths do not help.Use a shared loader and save normalized absolute import locations when extensions are registered. Workers use the same API to import the module basename from its saved parent directory. This supports relative and absolute paths,
./, trailing separators, spaces, path-like API arguments, and dotted module names while restoringsys.pathafter successful or failed imports.The CLI now delegates to
State.import_extensions(), resolves a relative-wbefore changing directory, and discovers extension settings checkers by the imported module's name. The Python API runner carries the state's working directory into workers and tolerates absent optional CLI injectables, allowingstate.run.all()to work when the process CWD differs from the model directory.imported_extensionsremains a list of importable module names for existing consumers such as SANDAG settings-checker discovery. Absolute locations are stored separately in private_extension_locationsworker metadata. The documentation explains this and Python's module-name caching behavior.Closes #1118.
Validation
Tests are integrated into the existing Core Testing workflow: the Linux foundation and macOS/Windows jobs discover the regressions through
pytest --pyargs activitysim.coreandpytest --pyargs activitysim.cli. The unchanged external SANDAG job validates compatibility with the example repository.imported_extensionsexactly as downstream settings checkers do, passing each module name toimportlib.import_module. Private worker locations do not alter that public registry.spawn.The original CLI multiprocessing regression was verified to fail on unfixed upstream.
Final verification on
38ba3c307: the full upstream Core Testing workflow passed, with all 16 test jobs successful (the branch-only documentation job was intentionally skipped). This includes Linux, macOS, Windows, every regional model, and the unchanged external SANDAG example. The SANDAG job passed all 34 tests, including the settings-checker tests that failed before the compatibility correction. Locally, all 39 extension regression cases and all 32 unchanged SANDAG settings-checker tests also passed. No changes were made to the SANDAG example repository.