refactor: reduce codebase complexity (events, observers, god classes)#114
Open
ElNiak wants to merge 2 commits into
Open
refactor: reduce codebase complexity (events, observers, god classes)#114ElNiak wants to merge 2 commits into
ElNiak wants to merge 2 commits into
Conversation
…d classes Track 1 - God class splits: - Split DockerBuilder into platform_detection, buildx_operations, image_operations - Split MetricsCollector into metric_types, metric_analysis - Split ExperimentManager into experiment_phases, experiment_cleanup Track 2 - Event/observer simplification: - Replace ~130 event subclasses with factory classmethods on 8 base classes - Simplify ITypedObserver from 881 to ~80 lines with dynamic dispatch - Remove all backward-compat alias functions from events - Clean up events/__init__.py from 404 to ~100 lines - Update all emitters to use factory methods directly Track 3 - Validator cleanup: - Remove dead validator exports from pydantic_factories.py - Simplify validators/__init__.py (remove __getattr__ machinery) Net reduction: ~5,600 lines across 40 files. 870 tests passing.
…, and mixin inlining Wire hidden core modules to CLI (report, build-metrics, debug commands), delete orphaned tools and unused mixins, inline single-use mixins into their consumers, replace empty protocol placeholders with intermediate base classes (ClientServerProtocolBase, PeerToPeerProtocolBase). Net: -1,746 LOC, 13 files deleted, 5 created, 10 new CLI commands.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors PANTHER to reduce complexity by consolidating event creation around factory methods, simplifying observer dispatch, inlining/removing legacy mixins, and splitting large components (Docker builder, metrics, protocol bases) into smaller modules. Adds CLI tooling for reporting/debugging/metrics.
Changes:
- Replace many event subclasses/usages with
*Event.<factory>()construction and simplify observer dispatch to(entity_type, name)routing. - Inline/remove several mixins and dead code (results service mixins, environment mixins, dependency resolver), and introduce protocol topology base classes.
- Split Docker builder responsibilities into new focused mixins and add new CLI commands (
report,debug,build-metrics).
Reviewed changes
Copilot reviewed 84 out of 85 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_core/test_event_system_integration.py | Updates tests to use event factory methods instead of concrete subclasses. |
| tests/unit/test_core/test_command_audit_observer.py | Updates observer tests/event creation to use ServiceEvent factories; adjusts expectations for supported types. |
| tests/integration/test_early_termination.py | Updates integration test to emit environment error via EnvironmentEvent.error(). |
| panther/webapp/services/results/test_data_mixin.py | Removes results-service test data mixin (dead/merged code path). |
| panther/webapp/services/results/analytics_mixin.py | Removes results-service analytics mixin. |
| panther/webapp/services/results/init.py | Simplifies package docstring after mixin removal. |
| panther/tools/plugins/external_dependency_resolver.py | Removes external dependency resolver module. |
| panther/tools/docs_gen/mkdocs/gen_ref_pages.py | Removes large commented-out reference generation block. |
| panther/plugins/services/service_event_mixin.py | Switches test-start emission to TestEvent.execution_started() factory. |
| panther/plugins/services/iut/quic/quinn/quinn.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin. |
| panther/plugins/services/iut/quic/quiche/quiche.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin. |
| panther/plugins/services/iut/quic/quic_go/quic_go.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/quant/quant.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/picoquic_shadow/picoquic_shadow.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/picoquic/picoquic.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/mvfst/mvfst.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/lsquic/lsquic.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/quic/aioquic/aioquic.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin. |
| panther/plugins/services/iut/minip/ping_pong/ping_pong.py | Replaces IUTManagerEventMixin with shared ServiceManagerEventMixin in MRO. |
| panther/plugins/services/iut/iut_event_mixin.py | Removes IUT facade event mixin. |
| panther/plugins/protocols/peer_to_peer/peer_to_peer.py | Adds peer-to-peer protocol base class with role validation semantics. |
| panther/plugins/protocols/peer_to_peer/init.py | Exports PeerToPeerProtocolBase. |
| panther/plugins/protocols/client_server/quic/quic.py | Makes QUIC protocol derive from ClientServerProtocolBase. |
| panther/plugins/protocols/client_server/minip/minip.py | Makes MiniP protocol derive from ClientServerProtocolBase; docstring tweaks. |
| panther/plugins/protocols/client_server/client_server.py | Adds client-server protocol base class with shared role/target semantics. |
| panther/plugins/protocols/client_server/init.py | Exports ClientServerProtocolBase. |
| panther/plugins/environments/network_environment_mixin.py | Removes network environment mixin. |
| panther/plugins/environments/network_environment/shadow_ns/shadow_ns.py | Removes unused import of ExperimentFinishedEarlyEvent. |
| panther/plugins/environments/network_environment/localhost_single_container/localhost_single_container.py | Removes unused import of ExperimentFinishedEarlyEvent. |
| panther/plugins/environments/execution_environment_mixin.py | Removes execution environment mixin. |
| panther/plugins/environments/execution_environment/base_execution_environment.py | Inlines mixin state/behavior and command-modification capability into base environment. |
| panther/plugins/environments/environment_plugin_mixin.py | Removes environment plugin base mixin (inlined elsewhere). |
| panther/core/utils/logging_mixin.py | Refactors logging mixin to own feature detection/logger initialization (no longer inherits FeatureLoggerMixin). |
| panther/core/utils/feature_logger_mixin.py | Reduces to a get_feature_logger() helper function. |
| panther/core/utils/init.py | Updates public API exports to remove FeatureLoggerMixin and keep get_feature_logger. |
| panther/core/observer/workflow/init.py | Docstring formatting cleanup. |
| panther/core/observer/impl/storage_observer.py | Simplifies imports and loosens typed handler signatures to BaseEvent. |
| panther/core/observer/impl/state_observer.py | Simplifies imports and loosens typed handler signatures to BaseEvent. |
| panther/core/observer/impl/metrics_observer.py | Simplifies imports and loosens typed handler signatures to BaseEvent. |
| panther/core/observer/impl/logger_observer.py | Simplifies imports and loosens typed handler signatures to BaseEvent. |
| panther/core/observer/impl/experiment_observer.py | Replaces class-based dispatch with (entity_type, name) dispatch and simplifies imports. |
| panther/core/observer/impl/command_audit_observer.py | Simplifies imports and loosens handler signatures to BaseEvent; changes supported-types behavior. |
| panther/core/metrics/resource_monitor.py | Docstring formatting cleanup. |
| panther/core/metrics/metric_types.py | Adds metric dataclasses extracted to a dedicated module. |
| panther/core/metrics/enums.py | Docstring formatting cleanup. |
| panther/core/events/test/emitter.py | Stops wildcard imports; uses TestEvent factories for many emissions. |
| panther/core/events/test/init.py | Narrows exports to core types/factories. |
| panther/core/events/step/events.py | Replaces most step subclasses with StepEvent factory classmethods. |
| panther/core/events/step/emitter.py | Emits step events via StepEvent factories. |
| panther/core/events/step/init.py | Narrows exports to core types/factories. |
| panther/core/events/service/emitter.py | Emits service events via ServiceEvent factories. |
| panther/core/events/service/init.py | Narrows exports to core types/factories. |
| panther/core/events/plugin/emitter.py | Emits plugin events via PluginEvent factories. |
| panther/core/events/plugin/init.py | Narrows exports to core types/factories. |
| panther/core/events/experiment/events.py | Converts most experiment events to ExperimentEvent factories; retains compat subclass(es). |
| panther/core/events/experiment/emitter.py | Emits experiment events via ExperimentEvent factories. |
| panther/core/events/experiment/init.py | Narrows exports to core types/factories. |
| panther/core/events/environment/emitter.py | Emits environment events via EnvironmentEvent factories. |
| panther/core/events/environment/init.py | Narrows exports to core types/factories. |
| panther/core/events/assertion/events.py | Replaces most assertion subclasses with AssertionEvent factory classmethods. |
| panther/core/events/assertion/emitter.py | Emits assertion events via AssertionEvent factories. |
| panther/core/events/assertion/init.py | Narrows exports to core types/factories. |
| panther/core/events/init.py | Shrinks exported surface to factory-based events and core types. |
| panther/core/docker_builder/platform_detection.py | Adds platform detection mixin for Docker builds. |
| panther/core/docker_builder/image_operations.py | Adds image build/ops mixin (buildx/caching integration). |
| panther/core/command_processor/mixins/modification_mixin.py | Removes command modification mixin (inlined elsewhere). |
| panther/core/command_processor/mixins/init.py | Removes export of CommandModificationMixin. |
| panther/core/command_processor/init.py | Removes export/reference to CommandModificationMixin. |
| panther/config/core/validators/init.py | Replaces lazy __getattr__ imports with direct imports/exports. |
| panther/cli/core/main.py | Registers new CLI commands: build-metrics, report, debug. |
| panther/cli/commands/report.py | Adds CLI commands for listing/showing/generating reports. |
| panther/cli/commands/debug.py | Adds CLI debugging/introspection commands for events/observers. |
| panther/cli/commands/build_metrics.py | Adds CLI for viewing/exporting builder/test metrics from JSONL storage. |
Comments suppressed due to low confidence (1)
panther/core/observer/impl/experiment_observer.py:1
- Defaulting
successtoFalsefor a"completed"test event will mark tests as failed in logs/observer state whenever the event payload doesn’t explicitly includesuccess. If"completed"semantically implies success in this event model, default toTruehere (or derive success from the event name/type and reserve failure for"execution_failed"/"failed"events).
"""Observer that tracks experiment lifecycle events and manages state."""
Comment on lines
29
to
35
| def __init__(self, *args, **kwargs): | ||
| """Initialize mixin state.""" | ||
| super().__init__(*args, **kwargs) | ||
| self._logger: Optional[logging.Logger] = None | ||
| self._log_context = {} | ||
| # Initialize feature-aware logging | ||
| self.__init_logger__() |
There was a problem hiding this comment.
LoggerMixin no longer inherits FeatureLoggerMixin, so super().__init__(*args, **kwargs) can now hit object.__init__ (or another base that doesn’t accept the forwarded parameters) and raise TypeError when any caller passes init args. To keep this mixin safe in multiple-inheritance chains, avoid forwarding unknown args here (e.g., call super().__init__() with no args, or defensively catch/strip args/kwargs before calling super()), and ensure the class still participates in cooperative initialization as intended.
Comment on lines
+249
to
+288
| selected_dockerfile = None | ||
| for candidate in buildkit_candidates: | ||
| if candidate.exists(): | ||
| selected_dockerfile = candidate | ||
| self.logger.debug( | ||
| "Selected Dockerfile (use_buildx=%s): %s", | ||
| use_buildx_config, | ||
| selected_dockerfile, | ||
| ) | ||
| break | ||
|
|
||
| if selected_dockerfile is None: | ||
| raise DockerBuildException( | ||
| message="No suitable Dockerfile found for regular build", | ||
| image_name=impl_name, | ||
| dockerfile=str(dockerfile_path), | ||
| build_error="Dockerfile not found", | ||
| ) | ||
|
|
||
| relative_dockerfile_path = selected_dockerfile.relative_to(context_path) | ||
|
|
||
| # Track build start time for cache | ||
| _build_start_time = time.time() | ||
|
|
||
| # Set the current dockerfile path for BuildKit detection | ||
| self._current_dockerfile_path = selected_dockerfile | ||
|
|
||
| # Check if we should use buildx for this build | ||
| if self._should_use_buildx(): | ||
| return self._build_with_buildx( | ||
| impl_name=impl_name, | ||
| dockerfile_path=dockerfile_path, | ||
| context_path=context_path, | ||
| build_args=build_args, | ||
| config=config, | ||
| version=version, | ||
| tag_version=tag_version, | ||
| experiment_id=experiment_id, | ||
| image_tag=image_tag, | ||
| ) |
There was a problem hiding this comment.
You compute selected_dockerfile (potentially choosing Dockerfile.buildkit) and even set self._current_dockerfile_path accordingly, but the buildx path still passes dockerfile_path=dockerfile_path (the original input), which can cause buildx builds to ignore the selected BuildKit-specific Dockerfile. Pass selected_dockerfile (and/or the derived relative_dockerfile_path) into _build_with_buildx so both build paths use the same Dockerfile selection.
Comment on lines
+56
to
+70
| if machine in ["arm64", "aarch64"]: | ||
| if not self._check_buildx_available(): | ||
| if not self._platform_detection_logged: | ||
| self.logger.warning( | ||
| "ARM64 architecture detected but Docker Buildx is not available. " | ||
| "Cross-platform builds may fail without Buildx support. " | ||
| "Set use_buildx: true to enable buildx for ARM64 builds." | ||
| ) | ||
| self.logger.warning( | ||
| "Detected ARM64 architecture '%s' -> using native platform: %s BUT expected errors may occur due to lack of support in some tools (picoTLS, z3, ivy) - set target_platform override if you want to force a different platform", | ||
| machine, | ||
| "linux/arm64", | ||
| ) | ||
| self._platform_detection_logged = True | ||
| docker_platform = "linux/arm64" |
There was a problem hiding this comment.
self._platform_detection_logged (and the various _cached_* attributes elsewhere in this mixin) are accessed directly but only documented as “expected on the host class”. If any adopter forgets to initialize them, this will raise AttributeError at runtime. Prefer using getattr(self, "_platform_detection_logged", False) (and similar for caches) or provide an __init__ in the mixin (with cooperative super()) that initializes these defaults.
|
|
||
| def get_supported_event_types(self) -> List[type]: | ||
| def get_supported_event_types(self) -> list: | ||
| """Return the list of event types this observer handles.""" |
There was a problem hiding this comment.
The docstring says this returns “the list of event types this observer handles”, but the implementation now always returns an empty list. If the typed-observer routing no longer uses this API, consider updating the docstring to explain the new contract (or removing/renaming the method if it’s no longer meaningful). If it is still used by registration/filtering, returning [] will prevent the observer from receiving events.
Suggested change
| """Return the list of event types this observer handles.""" | |
| """Return supported event types for this observer. | |
| This observer uses typed handler methods (for example, | |
| `handle_command_generation_started`, `handle_command_generated`, | |
| `handle_command_modified`) for event routing and does not rely on | |
| this method to register interest in events. It returns an empty list | |
| to satisfy the `ITypedObserver` interface while delegating routing | |
| to the typed handlers. | |
| Returns: | |
| An empty list, as event routing is based on handler method names | |
| rather than this registration hook. | |
| """ |
Comment on lines
+179
to
+201
| @click.option( | ||
| "--format", | ||
| "fmt", | ||
| type=click.Choice(["json", "markdown", "text", "all"]), | ||
| default="all", | ||
| help="Report format to generate (default: all)", | ||
| ) | ||
| @handle_errors | ||
| @pass_context_and_setup_logging | ||
| def generate(_ctx, experiment_dir, fmt): | ||
| r"""Generate reports from an experiment output directory. | ||
|
|
||
| \b | ||
| Examples: | ||
| panther report generate outputs/2024-01-01/exp1 | ||
| panther report generate outputs/2024-01-01/exp1 --format json | ||
| """ | ||
| from panther.core.reporting.experiment_reporter import ExperimentReporter | ||
|
|
||
| exp_path = Path(experiment_dir) | ||
| reporter = ExperimentReporter(exp_path, exp_path.name) | ||
|
|
||
| _info(f"Generating {fmt} report(s) for: {exp_path.name}") |
There was a problem hiding this comment.
The --format/--fmt option is accepted but never used; generate() always calls reporter.generate_reports() and prints all returned results. This makes the CLI option misleading and breaks expected behavior. Either pass fmt through to ExperimentReporter.generate_reports(...) (and filter which reports are generated), or remove the option.
Suggested change
| @click.option( | |
| "--format", | |
| "fmt", | |
| type=click.Choice(["json", "markdown", "text", "all"]), | |
| default="all", | |
| help="Report format to generate (default: all)", | |
| ) | |
| @handle_errors | |
| @pass_context_and_setup_logging | |
| def generate(_ctx, experiment_dir, fmt): | |
| r"""Generate reports from an experiment output directory. | |
| \b | |
| Examples: | |
| panther report generate outputs/2024-01-01/exp1 | |
| panther report generate outputs/2024-01-01/exp1 --format json | |
| """ | |
| from panther.core.reporting.experiment_reporter import ExperimentReporter | |
| exp_path = Path(experiment_dir) | |
| reporter = ExperimentReporter(exp_path, exp_path.name) | |
| _info(f"Generating {fmt} report(s) for: {exp_path.name}") | |
| @handle_errors | |
| @pass_context_and_setup_logging | |
| def generate(_ctx, experiment_dir): | |
| r"""Generate reports from an experiment output directory. | |
| \b | |
| Examples: | |
| panther report generate outputs/2024-01-01/exp1 | |
| """ | |
| from panther.core.reporting.experiment_reporter import ExperimentReporter | |
| exp_path = Path(experiment_dir) | |
| reporter = ExperimentReporter(exp_path, exp_path.name) | |
| _info(f"Generating reports for: {exp_path.name}") |
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.
Summary
Changes
Test plan
pytest tests/ -n auto -m unit— verify no regressionspytest tests/ -n auto -m integration— verify Docker-dependent tests