Move visualization import into method for Optimizer#810
Move visualization import into method for Optimizer#810rapids-bot[bot] merged 8 commits intoNVIDIA:developfrom
Conversation
Marked the agent optimizer task as complete in the README and improved LangChain callback registration to ensure fresh handlers are set in the current context. The updates reduce redundant hook registrations and enhance debugging clarity. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
Marked the agent optimizer task as complete in the README and improved LangChain callback registration to ensure fresh handlers are set in the current context. The updates reduce redundant hook registrations and enhance debugging clarity. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
The `optimizable_params` and `search_space` fields are now excluded from serialization. This prevents unnecessary data from being included in outputs, ensuring cleaner and more controlled serialization behavior. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
Moved the import of `create_pareto_visualization` to the function scope to optimize module loading and reduce unnecessary imports. This improves the efficiency and maintainability of the codebase. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
Add a specific warning for ImportError when visualization dependencies are not installed, guiding users to install `nvidia-nat-profiling`. This ensures clearer feedback during automated runs and improves debugging for missing libraries. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
WalkthroughMoved the top-level import of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PO as ParameterOptimizer
participant Viz as Pareto Visualizer (optional)
PO->>PO: Run parameter optimization
PO->>PO: attempt local import of create_pareto_visualization
alt import succeeds
PO->>Viz: call create_pareto_visualization(results)
Viz-->>PO: visualization completed
else ImportError
PO->>PO: log warning (recommend install nvidia-nat-profiling)
PO->>PO: continue without visualization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
/ok to test e4883da |
dnandakumar-nv
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/nat/profiler/parameter_optimization/parameter_optimizer.py (1)
133-151: Scope ImportError to the import and log stack traces per project rules.
- Catching ImportError across both import and execution can mislead if a different module is missing during plotting.
- Project guideline: when catching and not re-raising, use logger.exception to capture the stack trace.
Proposed refactor:
- try: - from nat.profiler.parameter_optimization.pareto_visualizer import create_pareto_visualization - logger.info("Generating Pareto front visualizations...") - create_pareto_visualization( - data_source=study, - metric_names=eval_metrics, - directions=directions, - output_dir=out_dir / "plots", - title_prefix="Parameter Optimization", - show_plots=False # Don't show plots in automated runs - ) - logger.info("Pareto visualizations saved to: %s", out_dir / "plots") - except ImportError as ie: - logger.warning("Could not import visualization dependencies: %s. " - "Have you installed nvidia-nat-profiling?", - ie) - except Exception as e: - logger.warning("Failed to generate visualizations: %s", e) + # Lazy import; visualization is optional. + try: + from nat.profiler.parameter_optimization.pareto_visualizer import create_pareto_visualization + except ImportError: + logger.exception( + "Pareto visualization disabled: optional dependencies are missing. " + "Install 'nvidia-nat-profiling' to enable plots." + ) + else: + try: + logger.info("Generating Pareto front visualizations...") + create_pareto_visualization( + data_source=study, + metric_names=eval_metrics, + directions=directions, + output_dir=out_dir / "plots", + title_prefix="Parameter Optimization", + show_plots=False, # Don't show plots in automated runs + ) + logger.info("Pareto visualizations saved to: %s", out_dir / "plots") + except Exception: + logger.exception("Failed to generate Pareto visualizations")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nat/profiler/parameter_optimization/parameter_optimizer.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/profiler/parameter_optimization/parameter_optimizer.py
🧬 Code graph analysis (1)
src/nat/profiler/parameter_optimization/parameter_optimizer.py (1)
src/nat/profiler/parameter_optimization/pareto_visualizer.py (1)
create_pareto_visualization(317-380)
⏰ 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). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (1)
src/nat/profiler/parameter_optimization/parameter_optimizer.py (1)
135-135: Lazy import to decouple optional viz dependency — LGTM.Moving the import inside the block prevents hard failures when users don’t need visualization.
Updated the mock path for `create_pareto_visualization` to reflect its relocation to `pareto_visualizer`. This ensures the test aligns with the updated module structure. Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
|
/ok to test 0784140 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/nat/profiler/test_parameter_optimizer.py (2)
203-211: Add a negative-path test for missing viz depsWe’re not exercising the ImportError path introduced by the lazy import. Add a test that simulates the viz import failing and asserts optimize_parameters still completes and writes artifacts.
Example test to append to this file:
def test_optimize_parameters_visualization_absent_deps(tmp_path: Path, monkeypatch): base_cfg = Config() out_dir = tmp_path / "opt_no_viz" optimizer_config = _make_optimizer_config(out_dir) optimizer_config.numeric.n_trials = 1 run_cfg = _make_run_config(base_cfg) best_params = {"lr": 0.01} def fake_apply_suggestions(_cfg: Config, suggestions: dict[str, object]) -> Config: # noqa: ANN001 return Config() def fake_create_study(directions: list[str]): # noqa: ANN001 return _FakeStudy(directions) # Make importing pareto_visualizer fail only when requested by parameter_optimizer import builtins as _bi real_import = _bi.__import__ def _failing_import(name, globals=None, locals=None, fromlist=(), level=0): # noqa: ANN001 if name.endswith(".pareto_visualizer"): raise ImportError("viz deps missing for test") return real_import(name, globals, locals, fromlist, level) with patch("nat.profiler.parameter_optimization.parameter_optimizer.apply_suggestions", side_effect=fake_apply_suggestions), \ patch("nat.profiler.parameter_optimization.parameter_optimizer.pick_trial", return_value=SimpleNamespace(params=best_params)), \ patch("nat.profiler.parameter_optimization.parameter_optimizer.optuna.create_study", side_effect=fake_create_study), \ patch("nat.profiler.parameter_optimization.parameter_optimizer.EvaluationRun", _DummyEvalRun), \ patch("builtins.__import__", side_effect=_failing_import): tuned = optimize_parameters(base_cfg=base_cfg, full_space={"lr": SearchSpace(low=0.001, high=0.1)}, optimizer_config=optimizer_config, opt_run_config=run_cfg) assert tuned is not None # Core artifacts still produced even when viz import fails assert (out_dir / "optimized_config.yml").exists() assert (out_dir / "trials_dataframe_params.csv").exists() # Optional: ensure plots dir not created when viz skipped assert not (out_dir / "plots").exists()
216-218: Remove redundant assert used to silence warningsThese variables are already used; the extra assert is unnecessary.
- # Silence unused warnings - assert apply_mock and pick_mock and viz_mock and eval_run_mock + # No-op: all patched mocks above are used; extra assert not needed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nat/profiler/test_parameter_optimizer.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/profiler/test_parameter_optimizer.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
tests/nat/profiler/test_parameter_optimizer.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/profiler/test_parameter_optimizer.py
🔇 Additional comments (1)
tests/nat/profiler/test_parameter_optimizer.py (1)
171-171: Correct patch target for visualization — LGTMSwitching the patch to nat.profiler.parameter_optimization.pareto_visualizer.create_pareto_visualization matches the new lazy import location and keeps the test stable.
|
/merge |
2 participants
Description
Moves matplotlib dependency into the parameter optimizer to prevent import errors if users are not using profiling
By Submitting this PR I confirm:
Summary by CodeRabbit