staged initialization of solvers#238
Conversation
|
Deployed test documentation to https://exasim-project.com/NeoFOAM/Build_PR_238 |
- Changed the method name from sort to solve in - Updated references to the sorting method in operations_dag.py, validator.py, and execution.py to use the new solve method. - Refactored initialization helper functions: renamed lazy and operator to init
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive 3-stage initialization framework and refactors graph utilities into a modular package structure. The changes establish a decorator-based initialization pattern (LOAD → RESOLVE → BUILD) for solver setup, and reorganize DAG-related functionality from a monolithic dag.py file into a well-structured graph/ package with separate modules for building, sorting, validation, and visualization.
Changes:
- Added new
initializationmodule implementing a 3-stage initialization API withStagedInit, helper functions, and execution pipeline - Refactored graph utilities from
dag.pyinto modulargraph/package with builder, validator, sorter, models, operations, and visualization modules - Updated all import references from
framework.dagtoframework.graphacross integration and component tests
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/neofoam/framework/initialization/* | New initialization framework with StagedInit, InitStep, helpers, execution pipeline, ConfigContext, and Depends |
| src/neofoam/framework/graph/* | Refactored graph utilities: builder, sorter, validator, models, operations_dag, visualization, and public API |
| src/neofoam/framework/dag.py | Deleted - functionality migrated to graph package |
| src/neofoam/framework/simulation.py | Updated import from dag to graph |
| src/neofoam/framework/solver_factory.py | Added SolverState dataclass (unused in this PR) |
| test/initialization/* | Comprehensive test suite for initialization framework |
| test/framework/integration/* | Updated imports from dag to graph |
| test/framework/components/* | Updated imports and added new graph module tests |
| doc/development/initialization.rst | New documentation for 3-stage initialization pattern |
| doc/index.rst | Added initialization section to table of contents |
| .gitignore | Added lib directory |
Comments suppressed due to low confidence (1)
src/neofoam/framework/graph/visualization.py:3
- The copyright year should be 2023 to match the rest of the codebase and the original file's copyright. Using 2026 is inconsistent with established files and appears to be a future date.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def add_initializer(self, name: str, value: Any) -> "InitializerBuilder": | ||
| """ | ||
| Add a initializer for mesh, domain, config, etc.). |
There was a problem hiding this comment.
The docstring says "Add a initializer for mesh, domain, config, etc.)." but there's a grammatical issue - it should be "Add an initializer for mesh, domain, config, etc." The opening parenthesis is also mismatched (no closing parenthesis follows logically).
| Add a initializer for mesh, domain, config, etc.). | |
| Add an initializer for mesh, domain, config, etc. |
| # SPDX-License-Identifier: GPL-3.0-or-later | ||
| # SPDX-FileCopyrightText: 2025 NeoFOAM authors | ||
|
|
||
| """ | ||
| Solver Factory - State management for solver initialization. | ||
| """ | ||
|
|
||
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
|
|
||
| @dataclass | ||
| class SolverState: | ||
| """ | ||
| Shared state container for solver initialization. | ||
|
|
||
| This dataclass stores state shared across the 3-stage initialization: | ||
| - core_models: Required models for the solver | ||
| - optional_models: Optional physics/turbulence models | ||
| - configs: Configuration dictionary | ||
| """ | ||
|
|
||
| core_models: list[Any] = field(default_factory=list) | ||
| optional_models: list[Any] = field(default_factory=list) | ||
| configs: dict[str, Any] = field(default_factory=dict) |
There was a problem hiding this comment.
The SolverState dataclass is introduced in this file but is not used anywhere in this PR. The PR description doesn't mention this file, and there's no clear connection to the initialization framework or graph utilities. This appears to be dead code that should either be removed or the PR description should explain its purpose.
| The framework uses explicit method calls instead of decorators: | ||
|
|
||
| Usage Example (Solver Initializer): | ||
| @dataclass | ||
| class MySolverInitializer: | ||
| def load(self) -> dict[str, Any]: | ||
| \"\"\"Load configuration from files.\"\"\" | ||
| algorithm = Algorithm.from_file("system/fvSolution") | ||
| return {"algorithm": algorithm} | ||
|
|
||
| def resolve(self, config: ConfigContext) -> None: | ||
| \"\"\"Configure inter-model dependencies.\"\"\" | ||
| for model in self.optional_models: | ||
| if hasattr(model, "resolve"): | ||
| model.resolve(config) | ||
|
|
||
| def build(self) -> list[InitStep]: | ||
| \"\"\"Create lazy initializers for runtime objects.\"\"\" | ||
| return [ | ||
| field("U", create=lambda ctx: create_vector_field(ctx["mesh"])), | ||
| model("transport", depends_on=["fields.U"], create=...), | ||
| ] | ||
|
|
||
| Usage Example (Model): | ||
| class MyModel(BaseModel): | ||
| def load(self) -> dict[str, Any]: | ||
| \"\"\"Load model configuration.\"\"\" | ||
| return {} | ||
|
|
||
| def resolve(self, config: ConfigContext) -> None: | ||
| \"\"\"Configure dependencies with other models.\"\"\" | ||
| pass | ||
|
|
||
| def build(self) -> list[InitStep]: | ||
| \"\"\"Create field initializers.\"\"\" | ||
| return [field("T", create=...)] | ||
|
|
||
| Execution: | ||
| from neofoam.framework.initialization.execution import execute_initialization | ||
|
|
||
| # In solver.initialize() | ||
| initializer = MySolverInitializer(argv) | ||
| config_items = initializer.load() | ||
|
|
||
| config = ConfigContext() | ||
| for key, value in config_items.items(): | ||
| config.register(key, value) | ||
|
|
||
| initializer.resolve(config) | ||
| lazy_inits = initializer.build() | ||
| ctx = execute_initialization(lazy_inits) |
There was a problem hiding this comment.
The module docstring describes "The framework uses explicit method calls instead of decorators" and shows examples using method-based approaches, but the actual implementation in staged_init.py uses decorators (@init.load, @init.resolve, @init.build). This is a significant discrepancy between the documentation and the implementation. Either update the docstring to match the decorator-based implementation, or clarify that both approaches are supported.
| known_names = set(name_counts) | ||
| for node_name, dependencies in dependencies_by_node.items(): | ||
| for dependency in dependencies: | ||
| # skip self-dependencies and duplicates, which are handled by other checks |
There was a problem hiding this comment.
The comment "skip self-dependencies and duplicates, which are handled by other checks" is misleading. The condition if dependency in known_names only skips dependencies that exist. Self-dependencies (where node_name == dependency) would pass this check and should be detected as cycles. However, they won't be caught here because they are in known_names. The comment should clarify that this check specifically handles missing dependencies, and cycles (including self-cycles) are detected later by the topological sort.
| # skip self-dependencies and duplicates, which are handled by other checks | |
| # only report dependencies that reference unknown nodes; cycles (including | |
| # self-dependencies) are detected later by the topological sort |
| # Wrap in a zero-arg closure; default-arg capture avoids late-binding (P-2.5) | ||
| self.initializers.append(init(name, create=lambda _ctx: value)) |
There was a problem hiding this comment.
Similar to _add_typed, if add_initializer is called multiple times in quick succession or in a loop, the lambda lambda _ctx: value will capture value by reference. Although each method call has its own scope, this can still be problematic if the caller is using this in a loop. Consider using a default argument to capture by value: lambda _ctx, v=value: v. The comment mentions "default-arg capture avoids late-binding (P-2.5)" but the implementation doesn't actually use default arguments.
|
|
||
|
|
||
| This ensures that configuration is loaded, dependencies are resolved, and runtime objects (fields, meshes) are built in the correct order. | ||
| To reduce the size of the solver code, the initialization logic is encapsulated in the ``StagedInit`` class and associated decorators that define each state in a separated file. |
There was a problem hiding this comment.
The text says "define each state in a separated file" but "state" should be "stage" to match the terminology used throughout the document (LOAD stage, RESOLVE stage, BUILD stage). Also, "separated" should be "separate".
| To reduce the size of the solver code, the initialization logic is encapsulated in the ``StagedInit`` class and associated decorators that define each state in a separated file. | |
| To reduce the size of the solver code, the initialization logic is encapsulated in the ``StagedInit`` class and associated decorators that define each stage in a separate file. |
| class GraphValidationReport: | ||
| """Validation report for a dependency graph.""" | ||
|
|
||
| diagnostics: Tuple[GraphDiagnostic, ...] |
There was a problem hiding this comment.
The GraphValidationReport should have a default value for diagnostics to allow creating an empty report easily. Consider changing to diagnostics: Tuple[GraphDiagnostic, ...] = () so that GraphValidationReport() creates a valid empty report without requiring explicit empty tuple.
| diagnostics: Tuple[GraphDiagnostic, ...] | |
| diagnostics: Tuple[GraphDiagnostic, ...] = () |
| - **Result**: A fully connected graph of model instances, verified and ready for deployment. | ||
|
|
||
| 3. **BUILD Stage (Construction)**: | ||
| - **Input**: The connected models and the mesh. |
There was a problem hiding this comment.
The documentation states "The connected models and the mesh" as inputs to the BUILD stage, but the actual implementation passes core_models and optional_models, not "mesh". The mesh is typically created during the BUILD stage as one of the InitSteps, not passed as input. This should be corrected to accurately reflect the implementation.
| - **Input**: The connected models and the mesh. | |
| - **Input**: The connected core and optional models. The mesh is created during this stage as one of the ``InitStep`` recipes, not passed in as input. |
|
|
||
| rect rgb(235, 255, 240) | ||
| Note over Manager, UserCode: 3. BUILD STAGE | ||
| Manager->>UserCode: Call @init.build(mesh) |
There was a problem hiding this comment.
The sequence diagram shows "@init.build(mesh)" being called with a mesh parameter, but looking at the actual implementation in staged_init.py, the build decorator signature is build(core_models: list[Any], optional_models: list[Any]), not build(mesh). The diagram should show "@init.build(core_models, optional_models)" to match the actual API.
HenningScheufler
changed the title
| @@ -0,0 +1,72 @@ | |||
| # SPDX-License-Identifier: GPL-3.0-or-later | |||
| # | |||
| # SPDX-FileCopyrightText: 2023 NeoFOAM authors | |||
There was a problem hiding this comment.
| # SPDX-FileCopyrightText: 2023 NeoFOAM authors | |
| # SPDX-FileCopyrightText: 2026 NeoFOAM authors |
| @@ -0,0 +1,217 @@ | |||
| # SPDX-License-Identifier: GPL-3.0-or-later | |||
| # | |||
| # SPDX-FileCopyrightText: 2023 NeoFOAM authors | |||
There was a problem hiding this comment.
| # SPDX-FileCopyrightText: 2023 NeoFOAM authors | |
| # SPDX-FileCopyrightText: 2026 NeoFOAM authors |
This pull request introduces a new modular graph utilities package under
src/neofoam/framework/graph, refactors DAG and graph-related logic into this package, and adds new abstractions for graph validation, construction, sorting, and visualization. The changes improve code organization, enable more robust validation and diagnostics, and clarify the initialization framework's API.Core framework and API improvements:
initializationmodule with clear staged initialization API, usage documentation, and explicit exports for all initialization-related utilities. This clarifies and documents the explicit 3-stage initialization approach.development/initializationsection.Graph utilities refactor and new features:
graphsubpackage containing modular files for graph building, sorting, validation, diagnostics, and visualization. This includes:builder.py: Utility for building dependency digraphs.models.py: Data models for graph diagnostics and validation reports.sorter.py: Abstractions and implementations for topological sorting.validator.py: Structured validation of dependency graphs, including duplicate detection, missing dependencies, and cycle detection.visualization.py: Refactored frompyvis_utils.py, now with improved naming and documentation for graph visualization. [1] [2]operations_dag.py: Contains DAG helpers for operation metadata, refactored from the previous monolithic implementation.__init__.py: Centralizes imports and defines the public API for the graph utilities package.Codebase cleanup and migration:
framework/dag.py, migrating and refactoring the relevant logic into the newgraph/operations_dag.pymodule.These changes collectively modularize and clarify the graph and initialization infrastructure, making the codebase easier to maintain and extend.