Conversation
This commit deletes the .cursorrules file, which contained guidelines and rules for Python development practices. The removal of this file helps streamline the project structure by eliminating unnecessary documentation that may no longer be relevant.
…ment results This commit introduces the `ResultAnalyzer` and `ExpAnalyzer` classes in the new `analysis.py` module, providing tools for analyzing and aggregating results from Hydra multirun experiments. It also updates the `mkdocs.yml` to include a new navigation entry for analysis documentation and adds corresponding markdown files for API documentation. Additionally, comprehensive tests for the analysis utilities are included to ensure functionality and reliability.
… logging This commit removes the `loguru` dependency from the project, replacing it with Python's built-in `logging` module for improved consistency and maintainability. The logging configuration is updated to support experiment-level and model-level logging, with new utilities for setting up loggers and handling different logging modes. Additionally, a new YAML configuration file is introduced for logging settings, and comprehensive tests are added to ensure the logging functionality works as expected.
…tox.ini This commit enhances the test-all and test-tox targets in the makefile by adding informative messages regarding the use of system Python interpreters for tox. It also includes a new step to install documentation dependencies for notebook tests. Additionally, the tox.ini file is updated with comments to clarify the requirement for system Python interpreters, improving the overall clarity and usability of the testing setup.
This commit updates the project version from 0.9.0 to 0.10.0 in both the pyproject.toml and uv.lock files. Additionally, it modifies the fallback version in abses/__init__.py to reflect the new version, ensuring consistency across the project.
📝 WalkthroughWalkthroughExperiment- and run-level logging were reworked: loguru removed in favor of Python logging; new logging utilities parse config (Hydra-compatible), determine per-run log paths/modes, and configure separate experiment logger. Analysis utilities for Hydra multiruns (ExpAnalyzer, ResultAnalyzer) and public API exports were added. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
abses/utils/datacollector.py (1)
216-218: Fix typo in warning message: missing space.There's a missing space between "defined" and "returning" in the warning message.
🔎 Proposed fix
if not self.model_reporters: logger.warning( - "No model reporters have been definedreturning empty DataFrame." + "No model reporters have been defined, returning empty DataFrame." )pyproject.toml (1)
50-57: Update constraint to exclude the vulnerable version or explicitly pin to a patched release.CVE-2025-5321 is confirmed to affect aim versions up to and including 3.29.1 (sandbox-escape/code-injection vulnerability in RestrictedPythonQuery). The current constraint
aim>=3.29.1allows the vulnerable version. While 3.30.0 stable does not exist, development versions (3.30.0.dev*) are available but unsuitable for production use. Stable 3.9.4 and 4.x releases exist; either specify a known patched version explicitly or wait for an official fix release and update the constraint toaim>=<patched-version>.abses/utils/log_config.py (2)
214-222:file_datefmtparameter is not passed to handler.Similarly,
file_datefmtis accepted (line 175) but not passed tocreate_file_handler(lines 215-221).🔎 Proposed fix
if file_path: handler = create_file_handler( filepath=file_path, level=file_level or level, fmt=file_format or DEFAULT_FORMAT, + datefmt=file_datefmt or DEFAULT_DATEFMT, rotation=rotation, retention=retention, )
297-308: Create new handlers for Mesa loggers instead of modifying shared references.Lines 300-301 modify formatter on handler objects that are the same references attached to
abses_logger(passed vialist(abses_logger.handlers)at line 422). This overwrites the original ABSES logger's formatter with the Mesa format, causing unintended side effects. Either copy handlers before modification or create fresh handlers for Mesa loggers.
🤖 Fix all issues with AI Agents
In @abses/core/experiment.py:
- Around line 160-172: The code mutates the input cfg when it's a plain dict
(cfg["outpath"] = ...), causing asymmetry with the DictConfig branch; to fix,
create a copy of the dict (e.g., cfg_copy = dict(cfg) or use copy.deepcopy) and
set cfg_copy["outpath"] = str(self.outpath) then call setup_exp_logger(cfg_copy)
instead of modifying cfg directly; keep the DictConfig branch unchanged
(OmegaConf.to_container/OmegaConf.create) and ensure you reference cfg and
setup_exp_logger in this change.
- Around line 421-446: The method _log_experiment_info currently prints a
hardcoded "Logging mode: separate"; instead read the actual logging mode from
the configuration passed in (cfg) and log that value (e.g., use cfg.logging.mode
or derive it from cfg.exp_file/related fields) so the output reflects the real
mode; if you prefer explicitness, add a logging_mode parameter to
_log_experiment_info and update all callers (places that call
_log_experiment_info) to pass the real mode so the logged line uses that
parameter rather than the hardcoded "separate".
In @abses/core/model.py:
- Around line 359-366: The isinstance check on exp_cfg prevents reading repeats
when self.settings returns an Omega DictConfig; update the check in the block
using logging_mode, self.run_id and exp_cfg so it accepts both dict and
DictConfig (e.g. isinstance(exp_cfg, (dict, DictConfig))) or remove the
isinstance guard entirely and directly call exp_cfg.get("repeats", 1), then pass
total_repeats into log_repeat_separator; ensure you import or reference
DictConfig if you add it to the isinstance tuple.
In @abses/utils/analysis.py:
- Around line 427-439: The results property yields a generator that gets
exhausted after one iteration; change it to build and cache a list (e.g.,
self._results_list) by iterating self.subdir and appending
ResultAnalyzer(subdir) while catching FileNotFoundError/ValueError and logging
as currently done, then return an iterator over that cached list (or expose the
list). Update diff_runs and agg_data to iterate over self._results_list (or call
list(self.results) once and reuse it) instead of iterating the generator
multiple times; also ensure nested loops in diff_runs use the cached list so
subsequent keys see the full set.
- Around line 162-170: The __init__ method in ResultAnalyzer contains a
duplicated docstring; remove the second misplaced docstring block that appears
after the call to super().__init__ so only the proper docstring (the one
directly under def __init__) remains and is the one parsed as documentation;
adjust surrounding whitespace/comments if needed to keep PEP8 formatting and
ensure ResultAnalyzer.__init__ still documents args and raises correctly.
In @abses/utils/log_config.py:
- Around line 206-211: The console_datefmt parameter accepted by the log
configuration is never forwarded to the console handler; update the call to
create_console_handler inside the block that checks `if console:` to pass the
`console_datefmt` argument (alongside level and fmt) so the handler uses the
user-provided date format; ensure the symbol names `create_console_handler`,
`console_datefmt`, and `logger.addHandler(handler)` are used to locate and
update the code.
In @abses/utils/log_parser.py:
- Around line 105-106: The current check on old_log is unsafe because subsequent
code calls old_log.get(); change the guard so old_log is guaranteed to be a dict
before using .get() — e.g., replace the conditional with a strict type check
that returns or normalizes non-dict values (for example: if not
isinstance(old_log, dict): return {} or set old_log = {}), ensuring any truthy
non-dict (like a string) cannot reach the .get() calls that follow.
- Around line 30-74: get_log_mode incorrectly treats an explicit log.mode="once"
the same as a missing value because OmegaConf.select(cfg, "log.mode",
default="once") cannot distinguish default vs explicit; update the DictConfig
branch to call OmegaConf.select(cfg, "log.mode") with no default and only if
that returns None set mode="once" and then consult OmegaConf.select(cfg,
"exp.logging.mode", default=None) for backward-compat; similarly update the
plain dict branch to detect explicit setting by checking if "log" in cfg and
"mode" in cfg["log"] (use that value if present, otherwise set mode="once" and
then inspect cfg.get("exp", {}).get("logging") for old_mode) and return the
explicit mode immediately without falling back to exp.logging.mode.
In @tests/utils/test_logging.py:
- Around line 52-136: All test methods in TestLogParser (e.g.,
test_get_log_mode_default, test_get_log_mode_from_config,
test_get_stdout_config_enabled, test_get_stdout_config_disabled,
test_get_file_config_enabled, test_get_file_config_defaults,
test_get_mesa_config, test_get_mesa_config_defaults) lack explicit return type
annotations; update each method signature to include the explicit return type
"-> None" (for example: def test_get_log_mode_default(self) -> None:) and apply
the same change to every other test method in the file.
- Around line 19-27: Add a TYPE_CHECKING import and make pytest-related imports
type-only: add "from typing import TYPE_CHECKING" at the top, and move or wrap
the "import pytest" (and any test fixture/plugin type imports) inside an "if
TYPE_CHECKING:" block so those imports are only for type checking; leave any
needed runtime imports (if decorators or runtime pytest APIs are used)
unchanged. Ensure you reference the existing import line "import pytest" and
wrap any plugin/fixture type imports similarly (e.g., mocker/fixture types)
under the TYPE_CHECKING guard.
🧹 Nitpick comments (15)
abses/utils/hydra_logging.py (2)
22-23: Remove unnecessary empty TYPE_CHECKING block.The
TYPE_CHECKINGblock contains onlypassand serves no purpose. It should be removed to reduce code clutter.🔎 Proposed fix
-if TYPE_CHECKING: - pass
38-49: Consider simplifying default value handling.Since
get_stdout_configalready applies defaults (or returns an empty dict), the conditional logic at lines 42-49 could be simplified. The function currently defines local defaults and applies them conditionally, which duplicates the default handling already performed byget_stdout_config.💡 Suggested simplification
- # Default configuration - default_format = "[%(asctime)s][%(name)s][%(levelname)s] - %(message)s" - default_datefmt = "%H:%M:%S" - default_level = "WARNING" - - if hydra_stdout: - format_str = hydra_stdout.get("format", default_format) - datefmt = hydra_stdout.get("datefmt", default_datefmt) - level = hydra_stdout.get("level", default_level) - else: - format_str = default_format - datefmt = default_datefmt - level = default_level + # Use defaults from get_stdout_config or fallback to reasonable defaults + format_str = hydra_stdout.get("format", "[%(asctime)s][%(name)s][%(levelname)s] - %(message)s") + datefmt = hydra_stdout.get("datefmt", "%H:%M:%S") + level = hydra_stdout.get("level", "WARNING")This relies on
get_stdout_configto provide configured values (with its own defaults already applied) and only uses the.get()fallbacks when the returned dict is empty.tests/utils/test_analysis.py (1)
21-22: Import pytest fixture types in the TYPE_CHECKING block.According to the coding guidelines, test files should conditionally import pytest fixtures and plugins under TYPE_CHECKING. Consider importing the fixture types used in this file:
🔎 Proposed improvement
if TYPE_CHECKING: - pass + from _pytest.fixtures import FixtureRequest + from _pytest.tmpdir import TempPathFactoryabses/conf/absespy.yaml (1)
70-76: Document placeholder values and their runtime behavior.The configuration includes several
nullplaceholders with example values in comments (rotation, retention, mesa level/format). Consider documenting:
- What happens when these values are
null(e.g., no rotation, inherit from parent level)- Validation of the example formats (e.g., "1 day", "100 MB" for rotation)
- Whether these can be dynamically configured at runtime
This will help users understand the configuration options better.
Based on coding guidelines, ensuring configuration is well-documented improves maintainability.
abses/utils/logging.py (1)
102-120: Consider refactoring to reduce parameter count.The
setup_model_loggerfunction now has 20+ parameters, which makes the signature complex and potentially error-prone to call. Consider refactoring options:
- Configuration object approach: Group related parameters into configuration objects (e.g.,
ConsoleConfig,FileConfig,MesaConfig)- Builder pattern: Use a builder class to construct logging configuration incrementally
- Kwargs dict: Accept structured configuration dictionaries for console/file/mesa settings
Example:
from dataclasses import dataclass @dataclass class ConsoleConfig: enabled: bool = True level: Optional[str] = None format: Optional[str] = None datefmt: Optional[str] = None def setup_model_logger( name: str = "model", level: str = "INFO", outpath: Optional[Path] = None, console: Union[bool, ConsoleConfig] = True, file: Optional[FileConfig] = None, mesa: Optional[MesaConfig] = None, ... ) -> tuple[logging.Logger, logging.Logger, logging.Logger]:This would improve readability and make it easier to extend configuration options in the future.
abses/utils/exp_logging.py (3)
35-36: Remove emptyTYPE_CHECKINGblock.The
TYPE_CHECKINGblock with onlypassserves no purpose and can be removed.🔎 Proposed fix
-if TYPE_CHECKING: - pass -
117-132: Extract duplicated outpath resolution logic into a helper function.The outpath resolution logic (handling dict vs DictConfig, None fallback, string/Path conversion) appears twice in this function (lines 117-132 and 154-169). This violates DRY and increases maintenance burden.
🔎 Proposed refactor
Add a helper function and use it in both places:
def _resolve_outpath(cfg: DictConfig | dict) -> Path: """Resolve output path from configuration. Args: cfg: Configuration dictionary. Returns: Resolved output path. """ if isinstance(cfg, dict): outpath = cfg.get("outpath") else: try: outpath = OmegaConf.select(cfg, "outpath", default=None) except Exception: outpath = None if outpath is None: return Path.cwd() elif isinstance(outpath, str): return Path(outpath) elif not isinstance(outpath, Path): return Path(str(outpath)) return outpathAlso applies to: 154-169
96-99: Bareexcept Exceptionswallows useful debugging info.The broad exception handling with just
exp_file_cfg_raw = {}makes it hard to debug configuration issues. Consider logging a warning or being more specific about expected exceptions.🔎 Proposed improvement
else: try: exp_file_cfg_raw = OmegaConf.select(cfg, "log.exp.file", default={}) - except Exception: + except (KeyError, TypeError) as e: + import logging + logging.getLogger(__name__).debug(f"Could not select log.exp.file: {e}") exp_file_cfg_raw = {}abses/core/model.py (2)
371-377: Remove empty conditional block with onlypass.The
if logging_mode == "separate"block contains only apassstatement and comments. This adds no functionality and reduces readability. Consider restructuring to only executesetup_logger_infowhen needed.🔎 Proposed refactor
# Display startup info - # In separate mode, setup_logger_info should only go to experiment log - # For model run logs, only log model-specific info - if logging_mode == "separate": - # In separate mode, don't log framework banner to model run log - # It will be logged to experiment log file instead - pass - else: - # In once/merge mode, log to model run log + # In separate mode, framework banner goes to experiment log only + # In once/merge mode, log to model run log + if logging_mode != "separate": setup_logger_info(self.exp) - # Always log model-specific info to model run log + self._logging_begin()
293-357: Consider extracting logging config extraction into a helper method.The
_setup_loggermethod has grown to handle many responsibilities: extracting stdout config, file config, mesa config, determining log mode, setting up handlers, and handling mode-specific behavior. Consider extracting the configuration extraction (lines 299-337) into a separate method like_get_logging_config()for better maintainability.abses/core/experiment.py (2)
163-169: Simplify nested type check.The
if isinstance(cfg_dict, dict)check on line 166 is redundant sinceOmegaConf.to_containerwithresolve=Truealways returns a dict when the input is a DictConfig.🔎 Proposed simplification
if isinstance(cfg, DictConfig): # Create a copy to avoid modifying original cfg_dict = OmegaConf.to_container(cfg, resolve=True) - if isinstance(cfg_dict, dict): - cfg_dict["outpath"] = str(self.outpath) # Convert Path to string - cfg_copy = OmegaConf.create(cfg_dict) - setup_exp_logger(cfg_copy) + cfg_dict["outpath"] = str(self.outpath) # Convert Path to string + cfg_copy = OmegaConf.create(cfg_dict) + setup_exp_logger(cfg_copy)
490-493: Remove empty conditional block.The
if logging_mode == "merge"block with only apassstatement and comment adds no value and reduces readability.🔎 Proposed fix
- # Log separator for merge mode - if logging_mode == "merge" and repeat_id > 1: - # Note: Separator will be logged in model setup - pass - # Get log file path for this repeatabses/utils/log_parser.py (1)
21-22: Remove emptyTYPE_CHECKINGblock.The
TYPE_CHECKINGblock with onlypassserves no purpose.🔎 Proposed fix
-if TYPE_CHECKING: - pass -abses/utils/analysis.py (2)
31-32: Remove emptyTYPE_CHECKINGblock.The
TYPE_CHECKINGblock with onlypassserves no purpose.🔎 Proposed fix
-if TYPE_CHECKING: - pass -
318-331:lru_cacheonget_datais ineffective.The
@lru_cache(maxsize=1)decorator caches the return value, but the method returnsself.data.copy(), meaning each call creates a new DataFrame copy. The cache stores a reference to the first copy, but subsequent calls still create new copies before the cache check.More importantly,
lru_cachedoesn't work correctly on instance methods becauseselfis part of the cache key.🔎 Proposed fix - Use cached_property or remove cache
- @lru_cache(maxsize=1) - def get_data(self, **kwargs: Any) -> pd.DataFrame: + def get_data(self, **kwargs: Any) -> pd.DataFrame: """Get processed data with optional transformations. This method can be overridden or extended to support different aggregation levels or data transformations. Args: **kwargs: Additional arguments for data processing. Returns: Processed DataFrame. """ return self.data.copy()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.cursor/rules/.cursorrulesabses/__init__.pyabses/agents/container.pyabses/conf/absespy.yamlabses/core/experiment.pyabses/core/model.pyabses/human/links.pyabses/space/patch.pyabses/utils/__init__.pyabses/utils/analysis.pyabses/utils/datacollector.pyabses/utils/exp_logging.pyabses/utils/hydra_logging.pyabses/utils/log_config.pyabses/utils/log_parser.pyabses/utils/logging.pyabses/utils/tracker/factory.pydocs/api/analysis.mdmakefilemkdocs.ymlpyproject.tomltest_colormap.pytests/utils/test_analysis.pytests/utils/test_logging.pytox.ini
💤 Files with no reviewable changes (1)
- test_colormap.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write detailed documentation using docstrings in all Python functions and classes, following PEP 257 conventions
Add typing annotations to each function or class in Python files, including explicit return types (including None where appropriate)
Files:
abses/agents/container.pyabses/utils/hydra_logging.pyabses/utils/log_parser.pytests/utils/test_logging.pyabses/utils/__init__.pyabses/utils/exp_logging.pyabses/space/patch.pyabses/utils/logging.pyabses/utils/tracker/factory.pyabses/human/links.pytests/utils/test_analysis.pyabses/utils/analysis.pyabses/core/experiment.pyabses/utils/datacollector.pyabses/core/model.pyabses/utils/log_config.pyabses/__init__.py
tests/**/@(test_*.py|*_test.py)
📄 CodeRabbit inference engine (.cursorrules)
tests/**/@(test_*.py|*_test.py): Use pytest for comprehensive testing in Python test files
In test files, import TYPE_CHECKING and conditionally import pytest fixtures and plugins (_pytest.capture.CaptureFixture, _pytest.fixtures.FixtureRequest, _pytest.logging.LogCaptureFixture, _pytest.monkeypatch.MonkeyPatch, pytest_mock.plugin.MockerFixture)
All tests should have typing annotations and contain docstrings
Files:
tests/utils/test_logging.pytests/utils/test_analysis.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Place all tests under ./tests directory and use pytest or pytest plugins exclusively
Files:
tests/utils/test_logging.pytests/utils/test_analysis.py
🧠 Learnings (2)
📚 Learning: 2025-12-12T14:30:01.950Z
Learnt from: CR
Repo: SongshGeoLab/ABSESpy PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-12T14:30:01.950Z
Learning: Applies to tests/**/@(test_*.py|*_test.py) : Use pytest for comprehensive testing in Python test files
Applied to files:
makefile
📚 Learning: 2025-12-12T14:30:01.950Z
Learnt from: CR
Repo: SongshGeoLab/ABSESpy PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-12T14:30:01.950Z
Learning: Implement robust error handling and logging with context capture in Python files
Applied to files:
abses/human/links.py
🧬 Code graph analysis (11)
abses/utils/hydra_logging.py (1)
abses/utils/log_parser.py (1)
get_stdout_config(149-174)
abses/utils/log_parser.py (4)
abses/core/experiment.py (2)
cfg(180-182)cfg(185-198)abses/space/patch.py (1)
select(595-637)abses/utils/analysis.py (2)
select(117-129)select(333-342)abses/human/links.py (2)
get(453-481)get(704-740)
tests/utils/test_logging.py (8)
abses/core/model.py (6)
MainModel(73-566)outpath(169-178)name(159-166)setup(534-535)step(537-541)exp(407-409)abses/core/experiment.py (6)
Experiment(143-601)cfg(180-182)cfg(185-198)outpath(243-247)new(207-226)batch_run(552-585)abses/utils/exp_logging.py (1)
setup_exp_logger(42-183)abses/utils/log_config.py (3)
create_console_handler(79-100)create_file_handler(103-162)determine_log_file_path(227-268)abses/utils/log_parser.py (4)
get_file_config(177-207)get_log_mode(30-74)get_mesa_config(210-230)get_stdout_config(149-174)abses/utils/logging.py (2)
setup_model_logger(102-179)formatter(40-51)abses/core/time_driver.py (1)
fmt(187-192)abses/core/job_manager.py (1)
ExperimentManager(20-150)
abses/utils/__init__.py (1)
abses/utils/analysis.py (2)
ExpAnalyzer(345-541)ResultAnalyzer(132-342)
abses/utils/exp_logging.py (2)
abses/utils/log_config.py (2)
create_console_handler(79-100)create_file_handler(103-162)abses/utils/log_parser.py (3)
get_file_config(177-207)get_log_mode(30-74)get_stdout_config(149-174)
abses/utils/logging.py (1)
abses/utils/log_config.py (2)
determine_log_file_path(227-268)setup_integrated_logging(339-426)
abses/utils/tracker/factory.py (1)
abses/core/model.py (1)
settings(420-432)
abses/utils/analysis.py (1)
abses/core/experiment.py (3)
_load_hydra_cfg(309-337)overrides(250-259)overrides(262-266)
abses/core/experiment.py (4)
abses/utils/exp_logging.py (1)
setup_exp_logger(42-183)abses/utils/log_parser.py (2)
get_file_config(177-207)get_log_mode(30-74)abses/utils/log_config.py (2)
determine_log_file_path(227-268)info(474-476)abses/utils/logging.py (1)
setup_logger_info(71-86)
abses/core/model.py (2)
abses/utils/log_parser.py (4)
get_file_config(177-207)get_log_mode(30-74)get_mesa_config(210-230)get_stdout_config(149-174)abses/utils/logging.py (3)
log_repeat_separator(89-99)setup_model_logger(102-179)setup_logger_info(71-86)
abses/__init__.py (2)
abses/core/model.py (1)
version(181-187)abses/core/protocols.py (1)
version(240-240)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: nbmake
🔇 Additional comments (26)
pyproject.toml (1)
12-12: LGTM - Version bump to 0.10.0.The version increment appropriately reflects the scope of changes in this PR, including the logging system migration and new analysis utilities.
abses/human/links.py (1)
13-13: LGTM - Standard logging migration.The migration from loguru to Python's standard logging follows idiomatic patterns: importing
loggingand creating a module-level logger withlogging.getLogger(__name__). This ensures proper logger hierarchy and compatibility with the centralized logging configuration introduced in this PR.Also applies to: 45-46
abses/agents/container.py (1)
14-14: LGTM - Logging migration correctly implemented.The standard logging setup follows the project-wide pattern. The debug log at line 294 appropriately tracks agent creation without being too verbose.
Also applies to: 46-48
abses/space/patch.py (1)
13-13: LGTM - Consistent logging pattern applied.The logging migration follows the same pattern as other modules. The logger is appropriately used for initialization info (line 277) and warning messages in the
apply_agentsmethod for exception handling.Also applies to: 58-59
tox.ini (1)
5-6: LGTM - Helpful documentation for interpreter selection.The comments clarify the expected Python interpreter source, which helps developers understand the tox configuration and avoid path issues with uv-managed environments.
abses/utils/datacollector.py (1)
12-12: LGTM - Logging migration correctly implemented.The standard logging pattern is correctly applied with the module-level logger positioned appropriately after type alias declarations.
Also applies to: 45-46
makefile (2)
191-198: LGTM - Improved test-all robustness.The changes add appropriate fallback handling for docs dependency installation and provide clear warnings about potential tox/uv compatibility issues. The direct
tox -p autoinvocation aligns with the guidance in tox.ini about using system Python interpreters.
203-204: LGTM - Consistent tox invocation pattern.The test-tox target now matches the approach used in test-all, with clear documentation about system Python requirements.
mkdocs.yml (1)
67-67: LGTM! Documentation navigation updated appropriately.The new Analysis API documentation is correctly added to the navigation structure under the "Workflow Control" section, which is an appropriate placement for experiment analysis utilities.
docs/api/analysis.md (1)
1-10: LGTM! Documentation structure is clean and follows conventions.The documentation file correctly references the new analysis utilities using mkdocstrings directives, and the YAML front matter is properly formatted.
abses/utils/tracker/factory.py (1)
228-228: LGTM! Parameter source correctly updated to model.settings.The change from
model.paramstomodel.settingsaligns with the broader refactoring toward centralized configuration management using Hydra. The type compatibility is maintained asmodel.settingsreturns aDictConfig, which matches the expected parameter type.abses/utils/__init__.py (1)
5-5: LGTM! New analysis utilities properly exposed in public API.The
ExpAnalyzerandResultAnalyzerclasses are correctly imported and added to__all__. Based on the provided code snippets, both classes follow the coding guidelines with comprehensive docstrings (PEP 257) and complete type annotations.Also applies to: 16-17
abses/utils/hydra_logging.py (1)
26-34: LGTM! Function documentation and type annotations comply with guidelines.The
generate_hydra_job_loggingfunction has proper docstrings following PEP 257 conventions and complete type annotations including explicit return types, meeting the project's coding guidelines.tests/utils/test_analysis.py (1)
25-321: Excellent test coverage for analysis utilities.The test suite comprehensively covers:
- Fixture setup with realistic Hydra multirun directory structures
- Base analyzer functionality (path, config, select, subdir)
- ResultAnalyzer initialization, data reading, and reporter extraction
- ExpAnalyzer overrides parsing, results generation, diff_runs, agg_data, and apply methods
- Edge cases like invalid paths and empty directories
The tests are well-structured with clear docstrings and proper type annotations.
abses/__init__.py (1)
35-42: Improved fallback version handling with warnings.The changes improve version handling by:
- Using Python's built-in
warningsmodule instead of environment variables- Updating the fallback version to v0.10.0-dev (aligns with PR objectives to bump from 0.9.0 to 0.10.0)
- Providing clear user feedback when package metadata is unavailable
This is a cleaner approach than the previous environment-variable based initialization.
abses/utils/logging.py (1)
89-100: Well-implemented separator logging function.The
log_repeat_separatorfunction properly:
- Has clear docstring and type annotations
- Uses the LoggerAdapter's
no_formatbinding for clean output- Provides useful visual separation for merge-mode runs
tests/utils/test_logging.py (1)
385-557: Comprehensive integration testing with good coverage.The
TestLoggingIntegrationclass provides excellent end-to-end testing:
- Tests experiment and model logging flows
- Validates file creation and handler configuration
- Covers all logging modes (once, separate, merge)
- Includes proper cleanup with ExperimentManager singleton reset
The integration tests effectively validate the complete logging system behavior.
abses/utils/exp_logging.py (1)
42-57: LGTM - Well-documented function signature.The function has proper type annotations, a clear docstring following PEP 257, and appropriate parameter documentation. As per coding guidelines, this follows the required documentation patterns.
abses/core/model.py (1)
46-58: LGTM - Clean import organization.The new imports for logging utilities are well-organized and follow the project's import structure. The addition of
log_repeat_separatorand parser functions aligns with the new logging subsystem.abses/core/experiment.py (1)
391-419: LGTM - Well-documented helper methods.The new helper methods
_get_logging_modeand_get_log_file_pathhave proper docstrings, type annotations, and follow PEP 257 conventions. The late import in_get_log_file_path(line 412) is acceptable to avoid circular imports.abses/utils/log_parser.py (2)
149-174: LGTM - Clean stdout configuration parser.The function correctly handles enabled/disabled states, applies sensible defaults, and has proper type annotations and docstring per coding guidelines.
177-207: LGTM - Well-structured file configuration parser.The function correctly defaults
enabledtoTruefor file logging (different from stdout'sFalsedefault), provides appropriate fallback names based on level, and includes all necessary configuration keys.abses/utils/analysis.py (1)
39-129: LGTM - Well-designed base analyzer class.
_BaseAnalyzerprovides a clean foundation with proper property setters for validation, good use ofcached_propertyforsubdir, and appropriate docstrings following PEP 257.abses/utils/log_config.py (3)
227-268: LGTM - Well-implemented log file path determination.The
determine_log_file_pathfunction correctly handles all three logging modes (once,separate,merge) with appropriate fallback behavior. The docstring clearly documents the expected behavior for each mode.
41-42: Good use of sentinel pattern for optional parameter.Using a sentinel object (
_LOG_FILE_PATH_DEFAULT) to distinguish between "not provided" and "explicitly None" is a clean pattern that avoids the common Python pitfall of using mutable defaults.
410-415: No action needed. The code correctly isolates experiment-level logging without interference.Lines 410-415 set
propagate=Falseon parent loggers without clearing handlers, which is the correct approach. The experiment logger (EXP_LOGGER_NAME = "abses.core.experiment") independently configures itself inexp_logging.pyby clearing handlers and setting up its own, so the redundantpropagate=Falsesetting here is harmless. Tests confirm experiment logging works as expected.
| def test_get_log_mode_default(self): | ||
| """测试获取默认日志模式""" | ||
| cfg = {} | ||
| assert get_log_mode(cfg) == "once" | ||
|
|
||
| def test_get_log_mode_from_config(self): | ||
| """测试从配置中获取日志模式""" | ||
| cfg = {"log": {"mode": "separate"}} | ||
| assert get_log_mode(cfg) == "separate" | ||
|
|
||
| cfg = {"log": {"mode": "merge"}} | ||
| assert get_log_mode(cfg) == "merge" | ||
|
|
||
| def test_get_stdout_config_enabled(self): | ||
| """测试获取启用的 stdout 配置""" | ||
| cfg = { | ||
| "log": { | ||
| "run": { | ||
| "stdout": { | ||
| "enabled": True, | ||
| "level": "DEBUG", | ||
| "format": "[%(levelname)s] %(message)s", | ||
| "datefmt": "%Y-%m-%d", | ||
| } | ||
| } | ||
| } | ||
| } | ||
| stdout_cfg = get_stdout_config(cfg, "run") | ||
| assert stdout_cfg["enabled"] is True | ||
| assert stdout_cfg["level"] == "DEBUG" | ||
| assert stdout_cfg["format"] == "[%(levelname)s] %(message)s" | ||
| assert stdout_cfg["datefmt"] == "%Y-%m-%d" | ||
|
|
||
| def test_get_stdout_config_disabled(self): | ||
| """测试获取禁用的 stdout 配置""" | ||
| cfg = {"log": {"run": {"stdout": {"enabled": False}}}} | ||
| stdout_cfg = get_stdout_config(cfg, "run") | ||
| assert stdout_cfg == {} | ||
|
|
||
| def test_get_file_config_enabled(self): | ||
| """测试获取启用的文件配置""" | ||
| cfg = { | ||
| "log": { | ||
| "run": { | ||
| "file": { | ||
| "enabled": True, | ||
| "name": "test_model", | ||
| "level": "WARNING", | ||
| "format": "[%(levelname)s] %(message)s", | ||
| "rotation": "1 day", | ||
| "retention": "10 days", | ||
| } | ||
| } | ||
| } | ||
| } | ||
| file_cfg = get_file_config(cfg, "run") | ||
| assert file_cfg["enabled"] is True | ||
| assert file_cfg["name"] == "test_model" | ||
| assert file_cfg["level"] == "WARNING" | ||
| assert file_cfg["rotation"] == "1 day" | ||
| assert file_cfg["retention"] == "10 days" | ||
|
|
||
| def test_get_file_config_defaults(self): | ||
| """测试文件配置的默认值""" | ||
| cfg = {"log": {"run": {"file": {"enabled": True}}}} | ||
| file_cfg = get_file_config(cfg, "run") | ||
| assert file_cfg["name"] == "model" # Default name for run | ||
| assert file_cfg["level"] == DEFAULT_LEVEL | ||
| assert file_cfg["format"] == DEFAULT_FORMAT | ||
| assert file_cfg["datefmt"] == DEFAULT_DATEFMT | ||
|
|
||
| def test_get_mesa_config(self): | ||
| """测试获取 Mesa 配置""" | ||
| cfg = {"log": {"run": {"mesa": {"level": "DEBUG", "format": "%(message)s"}}}} | ||
| mesa_cfg = get_mesa_config(cfg, "run") | ||
| assert mesa_cfg["level"] == "DEBUG" | ||
| assert mesa_cfg["format"] == "%(message)s" | ||
|
|
||
| def test_get_mesa_config_defaults(self): | ||
| """测试 Mesa 配置的默认值""" | ||
| cfg = {"log": {"run": {}}} | ||
| mesa_cfg = get_mesa_config(cfg, "run") | ||
| assert mesa_cfg["level"] is None | ||
| assert mesa_cfg["format"] is None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add return type annotations to all test methods.
According to the coding guidelines, all functions and methods in Python files must have explicit return types, including None for test methods. Currently, test methods in TestLogParser are missing return type annotations.
🔎 Proposed improvement
- def test_get_log_mode_default(self):
+ def test_get_log_mode_default(self) -> None:
"""测试获取默认日志模式"""
cfg = {}
assert get_log_mode(cfg) == "once"
- def test_get_log_mode_from_config(self):
+ def test_get_log_mode_from_config(self) -> None:
"""测试从配置中获取日志模式"""
cfg = {"log": {"mode": "separate"}}
assert get_log_mode(cfg) == "separate"Apply this pattern to all test methods in the file (lines 52-557).
As per coding guidelines, explicit return types are required for all functions and methods.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_get_log_mode_default(self): | |
| """测试获取默认日志模式""" | |
| cfg = {} | |
| assert get_log_mode(cfg) == "once" | |
| def test_get_log_mode_from_config(self): | |
| """测试从配置中获取日志模式""" | |
| cfg = {"log": {"mode": "separate"}} | |
| assert get_log_mode(cfg) == "separate" | |
| cfg = {"log": {"mode": "merge"}} | |
| assert get_log_mode(cfg) == "merge" | |
| def test_get_stdout_config_enabled(self): | |
| """测试获取启用的 stdout 配置""" | |
| cfg = { | |
| "log": { | |
| "run": { | |
| "stdout": { | |
| "enabled": True, | |
| "level": "DEBUG", | |
| "format": "[%(levelname)s] %(message)s", | |
| "datefmt": "%Y-%m-%d", | |
| } | |
| } | |
| } | |
| } | |
| stdout_cfg = get_stdout_config(cfg, "run") | |
| assert stdout_cfg["enabled"] is True | |
| assert stdout_cfg["level"] == "DEBUG" | |
| assert stdout_cfg["format"] == "[%(levelname)s] %(message)s" | |
| assert stdout_cfg["datefmt"] == "%Y-%m-%d" | |
| def test_get_stdout_config_disabled(self): | |
| """测试获取禁用的 stdout 配置""" | |
| cfg = {"log": {"run": {"stdout": {"enabled": False}}}} | |
| stdout_cfg = get_stdout_config(cfg, "run") | |
| assert stdout_cfg == {} | |
| def test_get_file_config_enabled(self): | |
| """测试获取启用的文件配置""" | |
| cfg = { | |
| "log": { | |
| "run": { | |
| "file": { | |
| "enabled": True, | |
| "name": "test_model", | |
| "level": "WARNING", | |
| "format": "[%(levelname)s] %(message)s", | |
| "rotation": "1 day", | |
| "retention": "10 days", | |
| } | |
| } | |
| } | |
| } | |
| file_cfg = get_file_config(cfg, "run") | |
| assert file_cfg["enabled"] is True | |
| assert file_cfg["name"] == "test_model" | |
| assert file_cfg["level"] == "WARNING" | |
| assert file_cfg["rotation"] == "1 day" | |
| assert file_cfg["retention"] == "10 days" | |
| def test_get_file_config_defaults(self): | |
| """测试文件配置的默认值""" | |
| cfg = {"log": {"run": {"file": {"enabled": True}}}} | |
| file_cfg = get_file_config(cfg, "run") | |
| assert file_cfg["name"] == "model" # Default name for run | |
| assert file_cfg["level"] == DEFAULT_LEVEL | |
| assert file_cfg["format"] == DEFAULT_FORMAT | |
| assert file_cfg["datefmt"] == DEFAULT_DATEFMT | |
| def test_get_mesa_config(self): | |
| """测试获取 Mesa 配置""" | |
| cfg = {"log": {"run": {"mesa": {"level": "DEBUG", "format": "%(message)s"}}}} | |
| mesa_cfg = get_mesa_config(cfg, "run") | |
| assert mesa_cfg["level"] == "DEBUG" | |
| assert mesa_cfg["format"] == "%(message)s" | |
| def test_get_mesa_config_defaults(self): | |
| """测试 Mesa 配置的默认值""" | |
| cfg = {"log": {"run": {}}} | |
| mesa_cfg = get_mesa_config(cfg, "run") | |
| assert mesa_cfg["level"] is None | |
| assert mesa_cfg["format"] is None | |
| def test_get_log_mode_default(self) -> None: | |
| """测试获取默认日志模式""" | |
| cfg = {} | |
| assert get_log_mode(cfg) == "once" | |
| def test_get_log_mode_from_config(self) -> None: | |
| """测试从配置中获取日志模式""" | |
| cfg = {"log": {"mode": "separate"}} | |
| assert get_log_mode(cfg) == "separate" | |
| cfg = {"log": {"mode": "merge"}} | |
| assert get_log_mode(cfg) == "merge" | |
| def test_get_stdout_config_enabled(self) -> None: | |
| """测试获取启用的 stdout 配置""" | |
| cfg = { | |
| "log": { | |
| "run": { | |
| "stdout": { | |
| "enabled": True, | |
| "level": "DEBUG", | |
| "format": "[%(levelname)s] %(message)s", | |
| "datefmt": "%Y-%m-%d", | |
| } | |
| } | |
| } | |
| } | |
| stdout_cfg = get_stdout_config(cfg, "run") | |
| assert stdout_cfg["enabled"] is True | |
| assert stdout_cfg["level"] == "DEBUG" | |
| assert stdout_cfg["format"] == "[%(levelname)s] %(message)s" | |
| assert stdout_cfg["datefmt"] == "%Y-%m-%d" | |
| def test_get_stdout_config_disabled(self) -> None: | |
| """测试获取禁用的 stdout 配置""" | |
| cfg = {"log": {"run": {"stdout": {"enabled": False}}}} | |
| stdout_cfg = get_stdout_config(cfg, "run") | |
| assert stdout_cfg == {} | |
| def test_get_file_config_enabled(self) -> None: | |
| """测试获取启用的文件配置""" | |
| cfg = { | |
| "log": { | |
| "run": { | |
| "file": { | |
| "enabled": True, | |
| "name": "test_model", | |
| "level": "WARNING", | |
| "format": "[%(levelname)s] %(message)s", | |
| "rotation": "1 day", | |
| "retention": "10 days", | |
| } | |
| } | |
| } | |
| } | |
| file_cfg = get_file_config(cfg, "run") | |
| assert file_cfg["enabled"] is True | |
| assert file_cfg["name"] == "test_model" | |
| assert file_cfg["level"] == "WARNING" | |
| assert file_cfg["rotation"] == "1 day" | |
| assert file_cfg["retention"] == "10 days" | |
| def test_get_file_config_defaults(self) -> None: | |
| """测试文件配置的默认值""" | |
| cfg = {"log": {"run": {"file": {"enabled": True}}}} | |
| file_cfg = get_file_config(cfg, "run") | |
| assert file_cfg["name"] == "model" # Default name for run | |
| assert file_cfg["level"] == DEFAULT_LEVEL | |
| assert file_cfg["format"] == DEFAULT_FORMAT | |
| assert file_cfg["datefmt"] == DEFAULT_DATEFMT | |
| def test_get_mesa_config(self) -> None: | |
| """测试获取 Mesa 配置""" | |
| cfg = {"log": {"run": {"mesa": {"level": "DEBUG", "format": "%(message)s"}}}} | |
| mesa_cfg = get_mesa_config(cfg, "run") | |
| assert mesa_cfg["level"] == "DEBUG" | |
| assert mesa_cfg["format"] == "%(message)s" | |
| def test_get_mesa_config_defaults(self) -> None: | |
| """测试 Mesa 配置的默认值""" | |
| cfg = {"log": {"run": {}}} | |
| mesa_cfg = get_mesa_config(cfg, "run") | |
| assert mesa_cfg["level"] is None | |
| assert mesa_cfg["format"] is None |
🤖 Prompt for AI Agents
In @tests/utils/test_logging.py around lines 52 - 136, All test methods in
TestLogParser (e.g., test_get_log_mode_default, test_get_log_mode_from_config,
test_get_stdout_config_enabled, test_get_stdout_config_disabled,
test_get_file_config_enabled, test_get_file_config_defaults,
test_get_mesa_config, test_get_mesa_config_defaults) lack explicit return type
annotations; update each method signature to include the explicit return type
"-> None" (for example: def test_get_log_mode_default(self) -> None:) and apply
the same change to every other test method in the file.
This commit modifies the pre-commit configuration to check the entire `abses/` directory, aligning local checks with CI behavior. Additionally, it specifies the Python interpreter path for the `py312` test environment in `tox.ini` to avoid conflicts with local installations. Changes in the `experiment.py` file enhance configuration handling by creating a copy of the input dictionary before modification, ensuring the original configuration remains unchanged. Other updates include improvements to logging and analysis utilities for better functionality and compatibility.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
abses/utils/log_config.py (2)
214-223: Missing file_datefmt parameter in file handler creation.The function accepts
file_datefmtparameter (line 175) but doesn't pass it tocreate_file_handler. This is the same issue that was previously fixed forconsole_datefmt.🔎 Proposed fix
# Add file handler if file_path: handler = create_file_handler( filepath=file_path, level=file_level or level, fmt=file_format or DEFAULT_FORMAT, + datefmt=file_datefmt or DEFAULT_DATEFMT, rotation=rotation, retention=retention, ) logger.addHandler(handler)
298-309: Handler formatters are being replaced, not cloned.When
handlersare provided, the code modifies the formatter on each handler directly (lines 300-302). This mutates the original handlers, which could cause unexpected side effects if those handlers are shared with other loggers.🔎 Proposed fix
Consider creating new handlers with the desired format instead of mutating existing ones, or document that handlers are modified in-place:
if handlers: - # Apply format to existing handlers + # Note: This modifies handlers in-place. Callers should pass copies if needed. formatter = logging.Formatter(format_str, datefmt=DATE_FORMAT) for handler in handlers: handler.setFormatter(formatter)Or clone handlers:
if handlers: formatter = logging.Formatter(format_str, datefmt=DATE_FORMAT) # Clone handlers to avoid mutating originals handlers = [copy.copy(h) for h in handlers] for handler in handlers: handler.setFormatter(formatter)
🤖 Fix all issues with AI Agents
In @abses/utils/analysis.py:
- Around line 181-191: The data property getter currently returns self._data
directly which can raise a vague AttributeError if _data hasn't been set; update
the data property (the @property def data) to check for the presence of
self._data (e.g., using hasattr or try/except AttributeError) and raise a clear
AttributeError with a descriptive message like "data has not been loaded; call
load_data(...) first" before returning the pd.DataFrame to match the docstring.
In @tests/utils/test_logging.py:
- Around line 49-56: All test methods (e.g.,
TestLogParser.test_get_log_mode_default) are missing explicit return type
annotations; update every test method in TestLogParser, TestLogConfig,
TestExpLogging, TestModelLogging, and TestLoggingIntegration to include the
return type -> None on their definitions so every function has an explicit
typing annotation per guidelines.
- Around line 19-27: Add a static-only import for TYPE_CHECKING and move any
test-fixture-specific imports into a conditional block so they are only imported
for type checking; specifically, add "from typing import TYPE_CHECKING" to the
top of tests/utils/test_logging.py and wrap pytest fixture imports (e.g., any
uses of pytest fixtures or fixture types) inside "if TYPE_CHECKING:" so runtime
import overhead is avoided while keeping type hints for tools.
🧹 Nitpick comments (6)
tests/utils/test_logging.py (1)
543-548: Add docstrings to inner TestModel class.The
TestModelclass defined inside the test lacks a docstring. While this is a minimal test fixture, adding a brief docstring improves clarity.🔎 Proposed fix
class TestModel(MainModel): + """Minimal test model for logging integration tests.""" + def setup(self): + """Setup method (no-op for testing).""" pass def step(self): + """Step method (no-op for testing).""" passabses/utils/log_parser.py (1)
21-22: Empty TYPE_CHECKING block is unused.The
TYPE_CHECKINGimport and block are present but empty. If no type-only imports are needed, this can be removed to reduce noise.🔎 Proposed fix
-from typing import TYPE_CHECKING, Any, Dict +from typing import Any, Dict from omegaconf import DictConfig, OmegaConf - -if TYPE_CHECKING: - passabses/utils/analysis.py (1)
31-32: Empty TYPE_CHECKING block is unused.Similar to log_parser.py, the
TYPE_CHECKINGimport and block are present but empty.🔎 Proposed fix
-from typing import TYPE_CHECKING, Any, Callable, Dict, Generator, List +from typing import Any, Callable, Dict, Generator, List ... -if TYPE_CHECKING: - passabses/utils/log_config.py (1)
22-23: Empty TYPE_CHECKING block is unused.The
TYPE_CHECKINGimport and block are present but empty across multiple files.abses/core/experiment.py (2)
483-498: Empty pass blocks suggest incomplete implementation.Lines 483-485 and 495-498 contain empty
passblocks with comments like "Note: This will be logged in the model's logger setup". These appear to be placeholders. Consider removing them or adding TODO comments if future work is planned.🔎 Proposed fix
- # For merge mode, log separator before first repeat - if logging_mode == "merge" and repeats > 1: - # Note: This will be logged in the model's logger setup - pass + # For merge mode, separators are logged in the model's logger setupAnd similarly for lines 495-498:
- # Log separator for merge mode - if logging_mode == "merge" and repeat_id > 1: - # Note: Separator will be logged in model setup - pass
401-421: Movedetermine_log_file_pathimport to module level.The
determine_log_file_pathfunction is imported inside_get_log_file_path(line 414), adding runtime overhead on each call. There is no circular dependency, so this import can be safely moved to the module level.🔎 Proposed fix
from abses.utils.exp_logging import EXP_LOGGER_NAME, setup_exp_logger +from abses.utils.log_config import determine_log_file_path from abses.utils.log_parser import get_file_config, get_log_mode ... def _get_log_file_path( self, log_name: str, repeat_id: int, logging_mode: str ) -> Optional[Path]: ... - from abses.utils.log_config import determine_log_file_path - return determine_log_file_path(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.pre-commit-config.yamlabses/core/experiment.pyabses/core/model.pyabses/utils/analysis.pyabses/utils/log_config.pyabses/utils/log_parser.pytests/utils/test_logging.pytox.ini
🚧 Files skipped from review as they are similar to previous changes (2)
- abses/core/model.py
- tox.ini
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/@(test_*.py|*_test.py)
📄 CodeRabbit inference engine (.cursorrules)
tests/**/@(test_*.py|*_test.py): Use pytest for comprehensive testing in Python test files
In test files, import TYPE_CHECKING and conditionally import pytest fixtures and plugins (_pytest.capture.CaptureFixture, _pytest.fixtures.FixtureRequest, _pytest.logging.LogCaptureFixture, _pytest.monkeypatch.MonkeyPatch, pytest_mock.plugin.MockerFixture)
All tests should have typing annotations and contain docstrings
Files:
tests/utils/test_logging.py
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write detailed documentation using docstrings in all Python functions and classes, following PEP 257 conventions
Add typing annotations to each function or class in Python files, including explicit return types (including None where appropriate)
Files:
tests/utils/test_logging.pyabses/core/experiment.pyabses/utils/log_parser.pyabses/utils/analysis.pyabses/utils/log_config.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Place all tests under ./tests directory and use pytest or pytest plugins exclusively
Files:
tests/utils/test_logging.py
🧠 Learnings (3)
📚 Learning: 2025-12-12T14:30:01.950Z
Learnt from: CR
Repo: SongshGeoLab/ABSESpy PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-12T14:30:01.950Z
Learning: Applies to tests/**/@(test_*.py|*_test.py) : In test files, import TYPE_CHECKING and conditionally import pytest fixtures and plugins (_pytest.capture.CaptureFixture, _pytest.fixtures.FixtureRequest, _pytest.logging.LogCaptureFixture, _pytest.monkeypatch.MonkeyPatch, pytest_mock.plugin.MockerFixture)
Applied to files:
tests/utils/test_logging.py
📚 Learning: 2025-12-12T14:30:01.950Z
Learnt from: CR
Repo: SongshGeoLab/ABSESpy PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-12T14:30:01.950Z
Learning: Applies to tests/**/@(test_*.py|*_test.py) : All tests should have typing annotations and contain docstrings
Applied to files:
tests/utils/test_logging.py
📚 Learning: 2025-12-12T14:30:01.950Z
Learnt from: CR
Repo: SongshGeoLab/ABSESpy PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-12T14:30:01.950Z
Learning: Applies to **/*.py : Add typing annotations to each function or class in Python files, including explicit return types (including None where appropriate)
Applied to files:
tests/utils/test_logging.py
🧬 Code graph analysis (4)
tests/utils/test_logging.py (8)
abses/core/model.py (5)
MainModel(73-567)outpath(169-178)name(159-166)setup(535-536)exp(408-410)abses/core/experiment.py (4)
Experiment(143-606)cfg(182-184)cfg(187-200)outpath(245-249)abses/utils/exp_logging.py (1)
setup_exp_logger(42-183)abses/utils/log_config.py (4)
create_console_handler(79-100)create_file_handler(103-162)determine_log_file_path(228-269)info(475-477)abses/utils/log_parser.py (4)
get_file_config(187-217)get_log_mode(30-84)get_mesa_config(220-240)get_stdout_config(159-184)abses/utils/logging.py (2)
setup_model_logger(102-179)formatter(40-51)abses/core/time_driver.py (1)
fmt(187-192)abses/core/job_manager.py (1)
ExperimentManager(20-150)
abses/core/experiment.py (3)
abses/utils/exp_logging.py (1)
setup_exp_logger(42-183)abses/utils/log_parser.py (2)
get_file_config(187-217)get_log_mode(30-84)abses/utils/log_config.py (2)
determine_log_file_path(228-269)info(475-477)
abses/utils/log_parser.py (2)
abses/core/experiment.py (2)
cfg(182-184)cfg(187-200)abses/utils/analysis.py (2)
select(117-129)select(325-334)
abses/utils/analysis.py (1)
abses/core/experiment.py (3)
_load_hydra_cfg(311-339)overrides(252-261)overrides(264-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (10)
.pre-commit-config.yaml (1)
29-31: LGTM! Mypy configuration now matches CI behavior.The change to check the entire
abses/directory ensures consistency between local pre-commit hooks and CI, which is valuable for catching type errors early. Note that this will run mypy on all files inabses/on every commit rather than just changed files, trading incremental speed for consistency.tests/utils/test_logging.py (1)
515-576: LGTM: Comprehensive integration test with proper cleanup.The
test_experiment_with_loggingtest properly:
- Saves and restores the singleton instance to avoid test pollution
- Uses try/finally to ensure handler cleanup (important for Windows)
- Verifies log file creation with helpful error messages
The singleton manipulation (lines 521-522) is acceptable for test isolation purposes.
abses/utils/log_parser.py (2)
30-84: LGTM: Backward compatibility logic correctly distinguishes explicit vs default values.The implementation now properly:
- Uses
OmegaConf.select(cfg, "log.mode")without a default to detect if the key exists (line 42)- Only falls back to
exp.logging.modewhenlog.modeis not explicitly set (lines 43-51)- Applies the same logic to plain dicts by checking
if "mode" in log_section(line 59)This addresses the previously flagged issue about being unable to distinguish explicit
log.mode="once"from a missing value.
107-147: LGTM: Fixed old_log type check.Line 115 now correctly guards against non-dict truthy values with
if not old_log or not isinstance(old_log, dict):. This prevents AttributeError whenold_logis a truthy non-dict (e.g., a string).abses/utils/analysis.py (2)
433-440: LGTM: Generator exhaustion bug fixed with cached property.The
_results_listcached property correctly caches the generator results, preventing the exhaustion issue whendiff_runsandagg_dataneed to iterate multiple times. This addresses the previously flagged critical bug.
304-308: IndexError is a legitimate exception for pd.read_csv with index_col and the exception handling is appropriate.
pd.read_csv(path, index_col=0)can raiseIndexErrorwhen the specified column index is out of range or when CSV parsing issues result in fewer columns than expected. The current exception handling pattern—attempting to read with an index column and falling back to reading without one—is a reasonable defensive approach for this scenario.abses/utils/log_config.py (1)
206-212: LGTM: console_datefmt is now passed to handler.The previously flagged issue about
console_datefmtnot being passed tocreate_console_handlerhas been fixed. Line 210 now correctly passesdatefmt=console_datefmt or DEFAULT_DATEFMT.abses/core/experiment.py (3)
160-174: LGTM: Input dict mutation issue fixed.The previously flagged issue about mutating the input
cfgdict is now fixed. Line 172 creates a shallow copy withcfg_copy = cfg.copy()before modifyingoutpath, ensuring the original input is not mutated.
423-451: LGTM: Logging mode is now dynamic.The previously hardcoded
"Logging mode: separate"has been replaced withf"Logging mode: {logging_mode}"on line 449, correctly reflecting the actual configuration.
505-511: Intentional use of print() for stdout-only output.The use of
print()instead ofloggeron line 511 is documented as intentional to avoid writing to model run log files. This is a valid design choice for user-facing progress information.
| @property | ||
| def data(self) -> pd.DataFrame: | ||
| """Raw data loaded from CSV or datacollector output. | ||
|
|
||
| Returns: | ||
| DataFrame containing the raw data. | ||
|
|
||
| Raises: | ||
| AttributeError: If data has not been loaded yet. | ||
| """ | ||
| return self._data |
There was a problem hiding this comment.
Missing AttributeError handling in data property getter.
The docstring mentions AttributeError: If data has not been loaded yet, but the getter simply returns self._data without handling the case where _data doesn't exist. This would raise an AttributeError but with an unclear message.
🔎 Proposed fix
@property
def data(self) -> pd.DataFrame:
"""Raw data loaded from CSV or datacollector output.
Returns:
DataFrame containing the raw data.
Raises:
AttributeError: If data has not been loaded yet.
"""
+ if not hasattr(self, "_data"):
+ raise AttributeError("Data has not been loaded yet. Call read_data() first.")
return self._data🤖 Prompt for AI Agents
In @abses/utils/analysis.py around lines 181 - 191, The data property getter
currently returns self._data directly which can raise a vague AttributeError if
_data hasn't been set; update the data property (the @property def data) to
check for the presence of self._data (e.g., using hasattr or try/except
AttributeError) and raise a clear AttributeError with a descriptive message like
"data has not been loaded; call load_data(...) first" before returning the
pd.DataFrame to match the docstring.
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from omegaconf import OmegaConf | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add TYPE_CHECKING import and conditional imports for pytest fixtures.
As per coding guidelines, test files should import TYPE_CHECKING and conditionally import pytest fixtures. This pattern improves type checking without runtime overhead.
🔎 Proposed fix
from __future__ import annotations
import logging
import tempfile
from pathlib import Path
+from typing import TYPE_CHECKING
import pytest
from omegaconf import OmegaConf
+
+if TYPE_CHECKING:
+ from _pytest.tmpdir import TempPathFactory🤖 Prompt for AI Agents
In @tests/utils/test_logging.py around lines 19 - 27, Add a static-only import
for TYPE_CHECKING and move any test-fixture-specific imports into a conditional
block so they are only imported for type checking; specifically, add "from
typing import TYPE_CHECKING" to the top of tests/utils/test_logging.py and wrap
pytest fixture imports (e.g., any uses of pytest fixtures or fixture types)
inside "if TYPE_CHECKING:" so runtime import overhead is avoided while keeping
type hints for tools.
| class TestLogParser: | ||
| """测试配置解析功能""" | ||
|
|
||
| def test_get_log_mode_default(self): | ||
| """测试获取默认日志模式""" | ||
| cfg = {} | ||
| assert get_log_mode(cfg) == "once" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add return type annotations to all test methods.
All test methods in the file are missing explicit -> None return type annotations. As per coding guidelines, all functions must have typing annotations including explicit return types.
🔎 Proposed fix pattern
Apply this pattern to all test methods in the file:
- def test_get_log_mode_default(self):
+ def test_get_log_mode_default(self) -> None:
"""测试获取默认日志模式"""This applies to all ~30 test methods across TestLogParser, TestLogConfig, TestExpLogging, TestModelLogging, and TestLoggingIntegration classes.
🤖 Prompt for AI Agents
In @tests/utils/test_logging.py around lines 49 - 56, All test methods (e.g.,
TestLogParser.test_get_log_mode_default) are missing explicit return type
annotations; update every test method in TestLogParser, TestLogConfig,
TestExpLogging, TestModelLogging, and TestLoggingIntegration to include the
return type -> None on their definitions so every function has an explicit
typing annotation per guidelines.
1 participant
Summary
This PR refactors the project structure and adds a new fire forest model example with comprehensive configuration management and experiment tracking.
Key Changes
uvfor dependency management and restructure project layoutTechnical Details
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.