Skip to content

Fix routing YAML best results export#1422

Open
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix/routing-yaml-best-results
Open

Fix routing YAML best results export#1422
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix/routing-yaml-best-results

Conversation

@fallintoplace

Copy link
Copy Markdown

Summary

Routing YAML export used dict.update with a set when best-results settings were enabled. That raises at runtime instead of writing best_result_path and best_result_interval.

This writes those fields with normal key assignment and treats None as the only missing interval value, so an explicit interval of 0 is preserved.

Validation

  • python3 -m py_compile python/cuopt/cuopt/routing/utils.py python/cuopt/cuopt/tests/routing/test_solver_settings.py
  • git diff --check

Not run locally: PYTHONPATH=python/cuopt uvx --with 'pytest<9.0' --with pyyaml --with numpy pytest python/cuopt/cuopt/tests/routing/test_solver_settings.py -k 'dump_config' -q could not collect because cudf is not installed in this local environment.

@fallintoplace fallintoplace requested a review from a team as a code owner June 10, 2026 21:39
@fallintoplace fallintoplace requested a review from tmckayus June 10, 2026 21:39
@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR fixes best result serialization logic in save_data_model_to_yaml by replacing a truthiness check with an explicit None guard, and adds test coverage that validates the behavior by dumping solver config and verifying the YAML output.

Changes

Best results serialization and validation

Layer / File(s) Summary
Best results None-check serialization
python/cuopt/cuopt/routing/utils.py
save_data_model_to_yaml now explicitly checks best_result_interval is not None before writing best_result_path and best_result_interval to YAML via direct key assignment, replacing the prior truthiness-based conditional and update() calls.
Test coverage for best results YAML output
python/cuopt/cuopt/tests/routing/test_solver_settings.py
test_dump_config is extended to import yaml, set best_results_file, call dump_best_results with interval 0, then load the generated YAML config file and assert that best_result_path and best_result_interval fields match the expected values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing a bug in the routing YAML export of best results settings.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem (dict.update error), the solution (key assignment with None checks), and validation performed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@python/cuopt/cuopt/tests/routing/test_solver_settings.py`:
- Around line 56-58: Tests use hardcoded filenames (config_file =
"solver_cfg.yaml", best_results_file = "best_results.txt") when calling
s.dump_config_file and s.dump_best_results which can collide in parallel runs;
change to use the pytest tmp_path fixture to create per-test files/paths (e.g.,
tmp_path / "solver_cfg.yaml" and tmp_path / "best_results.txt") and pass those
Path strings to s.dump_config_file and s.dump_best_results, and apply the same
tmp_path-based replacements for the other occurrences referenced around lines
72-75 so each test writes to isolated temporary files.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: aad36dbe-fcf2-4f80-ba50-bb871cf12d09

📥 Commits

Reviewing files that changed from the base of the PR and between c99a5ce and 7c734a8.

📒 Files selected for processing (2)
  • python/cuopt/cuopt/routing/utils.py
  • python/cuopt/cuopt/tests/routing/test_solver_settings.py

Comment on lines +56 to +58
best_results_file = "best_results.txt"
s.dump_config_file(config_file)
s.dump_best_results(best_results_file, 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use per-test temporary paths to avoid filename collisions.

Hardcoded solver_cfg.yaml / best_results.txt can cause flaky behavior in parallel or repeated test runs. Use tmp_path-scoped files.

Suggested change
-def test_dump_config():
+def test_dump_config(tmp_path):
     """Test SolverSettings solve with config file"""
     s = routing.SolverSettings()
-    config_file = "solver_cfg.yaml"
-    best_results_file = "best_results.txt"
+    config_file = tmp_path / "solver_cfg.yaml"
+    best_results_file = tmp_path / "best_results.txt"
     s.dump_config_file(config_file)
     s.dump_best_results(best_results_file, 0)
     assert s.get_config_file_name() == config_file
@@
-    with open(config_file) as f:
+    with open(config_file) as f:
         config = yaml.safe_load(f)
-    assert config["best_result_path"] == best_results_file
+    assert config["best_result_path"] == str(best_results_file)
     assert config["best_result_interval"] == 0

As per coding guidelines, **/*test*.{cpp,py,cu,cuh}: “Prevent flaky tests from GPU timing, uninitialized memory, or race conditions.”

Also applies to: 72-75

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cuopt/cuopt/tests/routing/test_solver_settings.py` around lines 56 -
58, Tests use hardcoded filenames (config_file = "solver_cfg.yaml",
best_results_file = "best_results.txt") when calling s.dump_config_file and
s.dump_best_results which can collide in parallel runs; change to use the pytest
tmp_path fixture to create per-test files/paths (e.g., tmp_path /
"solver_cfg.yaml" and tmp_path / "best_results.txt") and pass those Path strings
to s.dump_config_file and s.dump_best_results, and apply the same tmp_path-based
replacements for the other occurrences referenced around lines 72-75 so each
test writes to isolated temporary files.

Source: Coding guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant