Hotfixes issues at the beginning of some test routes - #871
Conversation
# Description Added the Autotest functionality for the ros 2 updated simulation Fixes # (861 - Broken Automatic Test) ## Type of change Please delete options that are not relevant. - New feature (non-breaking change which adds functionality) - This change requires a documentation update ## Does this PR introduce a breaking change? Restores the tests from ROS1 e.g. is old functionality not usable anymore ## Most important changes - added test folder with run_test.py and index_dict.py -added launch_leaderboard.test.sh to start the test from inside the container # Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works (might be obsolete with CI later on) - [ ] New and existing unit tests pass locally with my changes (might be obsolete with CI later on)
WalkthroughThis pull request introduces comprehensive CARLA leaderboard test infrastructure, including a new end-to-end test runner script (run_test.py) for evaluating autonomous agents, associated Bash helper scripts, test route/scenario XML definitions, test configuration data, and Docker development tooling updates to expose test functionality. Changes
Sequence DiagramsequenceDiagram
actor User
participant Docker as Docker Dev Env
participant Bash as launch_leaderboard<br/>.test.sh
participant CARLA as CARLA Server
participant Runner as test/run_test.py
participant Agent as Autonomous Agent
participant Stats as Statistics<br/>Manager
User->>Docker: leaderboard.test
Docker->>Bash: Execute script
Bash->>Bash: Reset ROS/Python environment
Bash->>Bash: Source leaderboard config
Bash->>CARLA: wait_for_carla.py (Poll readiness)
CARLA-->>Bash: Server Ready
Bash->>Runner: Launch test runner
Runner->>CARLA: Connect client
Runner->>CARLA: Load town map
Runner->>CARLA: Configure world (sync mode, traffic)
Runner->>Agent: Initialize agent + sensors
loop For Each Route
Runner->>CARLA: Load route waypoints
Runner->>CARLA: Start scenario
Runner->>Agent: Execute agent (with watchdog)
Agent->>Agent: Process sensor data & compute actions
Agent->>CARLA: Apply vehicle control
CARLA-->>Agent: Sensor observations
Runner->>Stats: Record metrics (collisions, infractions)
Runner->>CARLA: End scenario
end
Runner->>Stats: Finalize & aggregate results
Stats-->>Runner: Test report
Runner-->>User: Print results & exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (1)
code/test/run_test.py (1)
593-640: Consider refactoring repetitive infraction checks.The infraction checking blocks follow an identical pattern. This could be simplified with a helper function or loop.
infraction_checks = [ ("collisions_vehicle", "Vehicle"), ("collisions_pedestrian", "Pedestrian"), ("yield_emergency_vehicle_infractions", "emergency vehicle"), ("collisions_layout", "other collisions"), ("red_light", "Running Red light"), ("stop_infraction", "Running Stop sign"), ] for key, label in infraction_checks: print(Fore.CYAN + f"{label}:") if len(route_record.infractions[key]) > 0: print(route_record.infractions[key]) print(Fore.RED + "FAILED") flag = True else: print(Fore.GREEN + "SUCCEED")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build/docker/agent-ros2/scripts/devfunctions.bash(2 hunks)code/leaderboard_launcher/scripts/launch_leaderboard.test.sh(1 hunks)code/routes/routes_open_door.xml(1 hunks)code/routes/test.xml(6 hunks)code/test/index_dict.py(1 hunks)code/test/run_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-20T13:27:46.717Z
Learnt from: asamluka
Repo: una-auxme/paf PR: 624
File: code/routes/routes_bicycle.xml:55-57
Timestamp: 2025-01-20T13:27:46.717Z
Learning: In route configuration XMLs, scenarios (including ControlLoss) can be intentionally placed close together to test specific behaviors. There is no minimum spacing requirement between trigger points, as the spacing is determined by testing needs rather than distance rules.
Applied to files:
code/routes/routes_open_door.xmlcode/routes/test.xml
🧬 Code graph analysis (2)
code/leaderboard_launcher/scripts/launch_leaderboard.test.sh (1)
build/docker/agent-ros2/scripts/reset_env.bash (1)
reset_env(5-20)
code/test/run_test.py (3)
code/leaderboard_launcher/leaderboard_launcher/paf_agent_base.py (2)
sensors(17-89)destroy(91-92)code/planning/planning/behavior_agent/behavior_tree.py (1)
shutdown(368-370)code/perception/traffic_light_detection/src/traffic_light_inference.py (1)
parse_args(17-35)
🪛 Ruff (0.14.6)
code/test/run_test.py
1-1: Shebang is present but file is not executable
(EXE001)
167-167: Do not catch blind exception: Exception
(BLE001)
257-260: Create your own exception
(TRY002)
305-305: Do not catch blind exception: Exception
(BLE001)
321-321: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
326-326: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
368-368: Do not catch blind exception: Exception
(BLE001)
404-404: Do not catch blind exception: Exception
(BLE001)
421-421: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
code/routes/test.xml (1)
312-313: LGTM: Waypoint additions for route 1.The new starting waypoints are correctly structured and align with the PR objective of fixing route issues at the beginning of test routes.
build/docker/agent-ros2/scripts/devfunctions.bash (1)
66-72: LGTM: New leaderboard.test function follows existing patterns.The implementation is consistent with the existing
leaderboard.devfunction pattern, properly uses a subshell for isolation, and exports the function correctly.code/test/run_test.py (1)
321-326: Static analysis note:getattrusage is appropriate here.The static analysis flags
getattrwith constant strings, but in this context, the module is dynamically imported and may not have IDE-discoverable attributes. The current usage is acceptable for this dynamic agent loading pattern.
| #!/bin/bash | ||
| set -e | ||
|
|
||
| cd "${INTERNAL_WORKSPACE_DIR}" | ||
|
|
||
| # Reset full ROS/Python environment | ||
| source "${INTERNAL_WORKSPACE_DIR}/scripts/reset_env.bash" | ||
| reset_env | ||
|
|
||
| # Source the leaderboard workspace environment (excludes AGENT_DEPS_ROS_WS) | ||
| source "${INTERNAL_WORKSPACE_DIR}/env.leaderboard.bash" | ||
|
|
||
| # Source leaderboard specific venv | ||
| source leaderboard_venv/bin/activate | ||
|
|
||
| python3 "/workspace/code/leaderboard_launcher/leaderboard_launcher/wait_for_carla.py" | ||
|
|
||
| # Start leaderboard with arguments # edit "--routes" if you want a different testroute | ||
| exec python3 /workspace/code/test/run_test.py \ | ||
| --host="${CARLA_SIM_HOST}" \ | ||
| --debug=0 \ | ||
| --routes="/workspace/code/routes/test.xml" \ | ||
| --agent="/workspace/code/leaderboard_launcher/leaderboard_launcher/agent_deploy.py" \ | ||
| --track=MAP \ | ||
| "${@}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CARLA_SIM_HOST definitions in the codebase
rg -n "CARLA_SIM_HOST" -A 2 -B 2 --type bash --type dockerfile --type yaml --type ymlRepository: una-auxme/paf
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for CARLA_SIM_HOST definitions in the codebase
rg -n "CARLA_SIM_HOST" -A 2 -B 2Repository: una-auxme/paf
Length of output: 12856
Add validation for unset CARLA_SIM_HOST environment variable.
The script uses ${CARLA_SIM_HOST} directly without checking if it's set. While Docker compose files typically define this variable, the script should validate it or provide a fallback to match the defensive programming used elsewhere in the codebase (e.g., wait_for_carla.py uses os.getenv("CARLA_SIM_HOST", "localhost")).
Consider adding:
: ${CARLA_SIM_HOST:=localhost}after line 13 to ensure the variable has a default value, or validate it explicitly:
if [ -z "$CARLA_SIM_HOST" ]; then
echo "Error: CARLA_SIM_HOST is not set" >&2
exit 1
fi🤖 Prompt for AI Agents
In code/leaderboard_launcher/scripts/launch_leaderboard.test.sh around lines 1
to 25, the script uses ${CARLA_SIM_HOST} without ensuring it's set; add
defensive handling by either setting a default (e.g., default to "localhost"
when CARLA_SIM_HOST is unset) or explicitly validating and exiting with an error
if empty, then proceed to use the ensured variable for the python invocation.
| <position x="-269.4" y="5151.9" z="374"/> | ||
| <position x="-218.7" y="5271.0" z="373"/> | ||
| <position x="-163.9" y="5404.6" z="372"/> | ||
| <position x="-163.9" y="5404.6"<distance value="100"/> z="372"/> |
There was a problem hiding this comment.
Critical: Malformed XML syntax will cause parsing failure.
The <distance value="100"/> element is incorrectly embedded within the <position> tag's attributes. This is invalid XML and will fail to parse.
The <position> element in waypoints should be self-closing with only x, y, z attributes. If you intended to add a distance parameter, it should be part of a scenario definition, not a waypoint.
Apply this diff to fix the malformed XML:
- <position x="-163.9" y="5404.6"<distance value="100"/> z="372"/>
+ <position x="-163.9" y="5404.6" z="372"/>If the <distance value="100"/> was intentionally added for a specific purpose, please clarify the intended structure.
📝 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.
| <position x="-163.9" y="5404.6"<distance value="100"/> z="372"/> | |
| <position x="-163.9" y="5404.6" z="372"/> |
🤖 Prompt for AI Agents
In code/routes/routes_open_door.xml around line 46, the <position> tag is
malformed because the <distance value="100"/> element is embedded inside the
<position> start tag's attributes; remove the embedded <distance/> so the
waypoint's position is a proper tag with x, y, z attributes only (e.g.,
<position x="..." y="..." z="..."/>), and if the distance value is required move
it out as a separate element under the appropriate parent (e.g., a <distance
value="100"/> sibling or add a distance attribute to the scenario element) to
preserve valid XML structure.
| <scenario name="InvadingTurn_4" type="InvadingTurn"> | ||
| <trigger_point x="-557.5" y="4834.9" z="375.5" yaw="-179.9"/> | ||
| <distance value="120"/><position x="355.1" y="6138.1" z="358"/> | ||
| <position x="285.9" y="6127.4" z="358"/> | ||
| <position x="213.5" y="6100.5" z="358"/> | ||
| <position x="-221.6" y="6100.7" z="358"/> | ||
| <position x="-264.6" y="6019.4" z="360"/> | ||
| <position x="-297.9" y="5816.4" z="366"/> | ||
| <position x="-421.5" y="5871.9" z="367"/> | ||
| <position x="-374.0" y="6012.7" z="361"/> | ||
| <position x="-453.1" y="6101.2" z="360"/> | ||
| <position x="-536.9" y="6071.1" z="363"/> | ||
| <position x="-537.4" y="6000.8" z="365"/> | ||
| <position x="-620.9" y="6001.5" z="367"/> | ||
| <position x="-784.0" y="6185.4" z="365"/> | ||
| <position x="-817.1" y="6063.3" z="369"/> | ||
| <position x="-817.8" y="5685.4" z="375"/> | ||
| <position x="-819.3" y="4835.2" z="372"/> | ||
| <position x="-529.7" y="4730.9" z="375"/> | ||
| <position x="-497.3" y="4777.9" z="375"/> | ||
| <position x="-497.3" y="4884.0" z="375"/> | ||
| <position x="-496.1" y="5014.6" z="375"/> | ||
| <position x="-414.4" y="5056.2" z="375"/> | ||
| <position x="-222.5" y="4944.1" z="374"/> | ||
| <position x="-223.4" y="4834.8" z="374"/> | ||
| <position x="-586.4" y="4834.7" z="375"/> | ||
| <position x="-732.7" y="4962.2" z="376"/> | ||
| <position x="-705.4" y="5204.9" z="376"/> | ||
| <position x="-269.4" y="5151.9" z="374"/> | ||
| <position x="-218.7" y="5271.0" z="373"/> | ||
| <position x="-163.9" y="5404.6" z="372"/> | ||
| <position x="75.0" y="5587.0" z="367.2"/>" type="OppositeVehicleTakingPriority"> | ||
| </scenario> | ||
| <scenario name="OppositeVehicleTakingPriority_6" type="OppositeVehicleTakingPriority"> |
There was a problem hiding this comment.
Critical: Malformed XML structure in route 5 scenario.
Line 1773 has a <position> element incorrectly merged on the same line after <distance value="120"/>. This appears to be an editing error where a waypoint position was accidentally inserted into the scenario definition.
The <position> element belongs in the <waypoints> section, not inside a <scenario> element.
Apply this diff to fix the structure:
<scenario name="InvadingTurn_4" type="InvadingTurn">
<trigger_point x="-557.5" y="4834.9" z="375.5" yaw="-179.9"/>
- <distance value="120"/><position x="355.1" y="6138.1" z="358"/>
+ <distance value="120"/>
</scenario>If the position x="355.1" y="6138.1" z="358" was intended to be added as a waypoint, it should be added to the <waypoints> section of route 5 (around line 1513-1517).
📝 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.
| <scenario name="InvadingTurn_4" type="InvadingTurn"> | |
| <trigger_point x="-557.5" y="4834.9" z="375.5" yaw="-179.9"/> | |
| <distance value="120"/><position x="355.1" y="6138.1" z="358"/> | |
| <position x="285.9" y="6127.4" z="358"/> | |
| <position x="213.5" y="6100.5" z="358"/> | |
| <position x="-221.6" y="6100.7" z="358"/> | |
| <position x="-264.6" y="6019.4" z="360"/> | |
| <position x="-297.9" y="5816.4" z="366"/> | |
| <position x="-421.5" y="5871.9" z="367"/> | |
| <position x="-374.0" y="6012.7" z="361"/> | |
| <position x="-453.1" y="6101.2" z="360"/> | |
| <position x="-536.9" y="6071.1" z="363"/> | |
| <position x="-537.4" y="6000.8" z="365"/> | |
| <position x="-620.9" y="6001.5" z="367"/> | |
| <position x="-784.0" y="6185.4" z="365"/> | |
| <position x="-817.1" y="6063.3" z="369"/> | |
| <position x="-817.8" y="5685.4" z="375"/> | |
| <position x="-819.3" y="4835.2" z="372"/> | |
| <position x="-529.7" y="4730.9" z="375"/> | |
| <position x="-497.3" y="4777.9" z="375"/> | |
| <position x="-497.3" y="4884.0" z="375"/> | |
| <position x="-496.1" y="5014.6" z="375"/> | |
| <position x="-414.4" y="5056.2" z="375"/> | |
| <position x="-222.5" y="4944.1" z="374"/> | |
| <position x="-223.4" y="4834.8" z="374"/> | |
| <position x="-586.4" y="4834.7" z="375"/> | |
| <position x="-732.7" y="4962.2" z="376"/> | |
| <position x="-705.4" y="5204.9" z="376"/> | |
| <position x="-269.4" y="5151.9" z="374"/> | |
| <position x="-218.7" y="5271.0" z="373"/> | |
| <position x="-163.9" y="5404.6" z="372"/> | |
| <position x="75.0" y="5587.0" z="367.2"/>" type="OppositeVehicleTakingPriority"> | |
| </scenario> | |
| <scenario name="OppositeVehicleTakingPriority_6" type="OppositeVehicleTakingPriority"> | |
| <scenario name="InvadingTurn_4" type="InvadingTurn"> | |
| <trigger_point x="-557.5" y="4834.9" z="375.5" yaw="-179.9"/> | |
| <distance value="120"/> | |
| </scenario> | |
| <scenario name="OppositeVehicleTakingPriority_6" type="OppositeVehicleTakingPriority"> |
🤖 Prompt for AI Agents
In code/routes/test.xml around lines 1771 to 1775, the <position x="355.1"
y="6138.1" z="358"/> element was accidentally placed inline inside the
<scenario> block after <distance value="120"/>, producing malformed XML; remove
that <position/> from the scenario so the <scenario name="InvadingTurn_4" ...>
contains only its valid child elements (trigger_point and distance) and ensure
the scenario is properly closed, then add the removed <position x="355.1"
y="6138.1" z="358"/> as a new <position> entry under the <waypoints> section of
route 5 (around lines 1513-1517) so the waypoint is correctly associated with
the route.
| # list_structure: [id_number, scenario_name, time in seconds] | ||
| TEST_THRESHOLD_AND_NAME = [ | ||
| [0, "routes_car_in_lane", 100.0], | ||
| [1, "routes_construction_sign", 100.0], | ||
| [2, "routes_construction_sign2", 100.0], | ||
| [3, "routes_curve", 100.0], | ||
| [4, "routes_firetruck_crash", 100.0], | ||
| [5, "routes_intersection_pedestrian", 100.0], | ||
| [6, "routes_intersection_stop_sign", 100.0], | ||
| [7, "routes_open_door", 100.0], | ||
| [8, "routes_pedestrian_behind_bus", 100.0], | ||
| [9, "routes_stop_sign_pedestrian_cross", 100.0], | ||
| [10, "bicycle", 100.0], | ||
| ] |
There was a problem hiding this comment.
Naming inconsistency: "bicycle" lacks "routes_" prefix.
Entry at index 10 uses "bicycle" while all other entries use the "routes_" prefix (e.g., "routes_car_in_lane"). This inconsistency could cause issues if lookups are performed by name pattern matching.
- [10, "bicycle", 100.0],
+ [10, "routes_bicycle", 100.0],Additionally, consider using a more descriptive data structure (e.g., named tuples or a dataclass) for better maintainability and type safety.
🤖 Prompt for AI Agents
In code/test/index_dict.py around lines 1 to 14, the entry at index 10 uses the
name "bicycle" which is inconsistent with the "routes_" prefix used by all other
scenario names; change "bicycle" to "routes_bicycle" to match the naming pattern
and avoid lookup/matching errors, and optionally refactor
TEST_THRESHOLD_AND_NAME to a clearer structure (e.g., namedtuple or dataclass
with fields id, name, threshold) for improved maintainability and type safety.
| import traceback | ||
| import argparse | ||
| from argparse import RawTextHelpFormatter | ||
| from distutils.version import LooseVersion |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Python version requirements and distutils usage
echo "=== Finding requirements/setup files ==="
fd -e txt -e cfg -e ini -e toml -e yaml -e yml | grep -i 'req\|setup\|pyproject\|pipfile' | head -20
echo ""
echo "=== Checking line 19 and line 93 context in run_test.py ==="
cat -n code/test/run_test.py | sed -n '15,25p'
cat -n code/test/run_test.py | sed -n '88,98p'
echo ""
echo "=== Checking for other distutils usage ==="
rg 'distutils' -n code/test/run_test.pyRepository: una-auxme/paf
Length of output: 1745
🏁 Script executed:
#!/bin/bash
echo "=== Searching for packaging library imports ==="
rg 'from packaging|import packaging' -n
echo ""
echo "=== Checking Python version in setup/config files ==="
fd -e txt -e cfg -e toml | xargs grep -l 'python_requires\|Programming Language :: Python' 2>/dev/null | head -5Repository: una-auxme/paf
Length of output: 159
🌐 Web query:
distutils deprecation removal Python 3.12
💡 Result:
Short answer: distutils was deprecated by PEP 632 (Python 3.10) and removed from the CPython standard library in Python 3.12 — it is no longer installed by default. [1][2]
What to do
- For most code: install and use setuptools (it provides a compatibility copy of distutils) or change imports to setuptools equivalents. Example: pip install --upgrade setuptools. [1][3]
- For build-system migration: move to a PEP 517/518-backed build backend (setuptools, flit, poetry, hatch, scikit-build-core, meson-python, etc.). See PEP 632 migration advice. [1]
- Note: the last Python that shipped distutils in the stdlib was 3.11; code that imports distutils will fail on 3.12 unless you install a replacement. [2][3]
Sources
- PEP 632 — Deprecate distutils module.
- Python stdlib docs: distutils — deprecated and removed (Python 3.12).
- What’s New in Python 3.12 (removal note).
🏁 Script executed:
#!/bin/bash
echo "=== Checking main requirements files for packaging library ==="
cat code/requirements.txt | grep -i packaging
echo ""
echo "=== Checking all requirements files ==="
grep -r 'packaging' code/requirements*.txt build/docker/*/setup-scripts/requirements*.txt 2>/dev/null
echo ""
echo "=== Checking setup.cfg files in code/ ==="
grep -r 'install_requires\|requires' code/*/setup.cfg | head -20Repository: una-auxme/paf
Length of output: 203
🌐 Web query:
packaging.version.Version comparison LooseVersion compatibility
💡 Result:
Short answer: packaging.version.Version implements PEP 440 ordering and is the recommended, stricter comparator for Python package versions; distutils.version.LooseVersion uses a very different, “looser” tokenized comparison for arbitrary strings and is not compatible with PEP 440 semantics. Use packaging.Version (or packaging.parse + specifiers) for package/version handling; use LooseVersion only when you must compare non‑PEP‑440, heterogeneous version strings. [1][2]
Key points and practical effects:
- packaging.version.Version strictly enforces PEP 440; invalid PEP‑440 strings raise InvalidVersion (packaging.parse historically returns a Version for PEP‑440 and a LegacyVersion for nonconforming strings). It also implements the PEP‑440 ordering rules for pre‑/post/dev/local segments. [1][3]
- distutils.version.LooseVersion tokenizes by numbers/strings and compares component lists (more permissive, different ordering rules), so results can differ from PEP‑440 ordering and it won’t honor PEP‑440 rules for pre/post/dev/local versions. Distutils is deprecated; LooseVersion may be provided separately by third‑party packages. [2][4]
- Recommendation: if you’re comparing Python package versions or using specifiers, migrate to packaging (PEP 440). If you must interoperate with arbitrary nonstandard version schemes (not PEP‑440), LooseVersion-like logic may be more tolerant but is less predictable for package dependency semantics. [1][2][3]
References
- packaging (PEP 440 / packaging docs). [PEP 440 / packaging docs]. [3]
- Stack Overflow discussion recommending packaging.Version and noting LooseVersion/distutils deprecation. [2]
- Python Packaging User Guide — version specifiers & differences from legacy parsing. [1]
- Notes about LooseVersion forks / packaging guidance (e.g., Ubuntu/looseversion and PyPI looseversion). [4]
🏁 Script executed:
#!/bin/bash
python3 << 'EOF'
# Test version string validity
import sys
version_string = "0.9.10"
# Test with packaging.version.Version
try:
from packaging.version import Version
v = Version(version_string)
print(f"packaging.version.Version('{version_string}'): SUCCESS - {v}")
print(f" Is PEP 440 compliant: Yes")
except Exception as e:
print(f"packaging.version.Version('{version_string}'): FAILED - {e}")
# Test with distutils.version.LooseVersion
try:
from distutils.version import LooseVersion
v = LooseVersion(version_string)
print(f"\ndistutils.version.LooseVersion('{version_string}'): SUCCESS - {v}")
except Exception as e:
print(f"\ndistutils.version.LooseVersion('{version_string}'): FAILED - {e}")
# Test comparison
try:
from packaging.version import Version
v1 = Version("0.9.9")
v2 = Version("0.9.10")
print(f"\npackaging.version comparison: {v1} < {v2} = {v1 < v2}")
except Exception as e:
print(f"\npackaging.version comparison: FAILED - {e}")
print(f"\nPython version: {sys.version}")
EOFRepository: una-auxme/paf
Length of output: 143
🌐 Web query:
migrate distutils.version.LooseVersion to packaging library CARLA leaderboard
💡 Result:
Short answer: replace LooseVersion with packaging.version (or, if you must keep LooseVersion semantics for non‑PEP440 strings, install the separate looseversion package). Use packaging.parse/Version for PEP 440–compliant comparisons; use looseversion.LooseVersion as a drop‑in if you need the old, permissive parsing. See citations below. (packaging.pypa.io)
Suggested changes (example):
-
Old (distutils.LooseVersion)
from distutils.version import LooseVersion
if LooseVersion(a) < LooseVersion(b):
... -
New (preferred when strings follow PEP 440)
from packaging.version import parse # or from packaging.version import Version
if parse(a) < parse(b):
...or, if you want explicit Version and want InvalidVersion to raise:
from packaging.version import Version, InvalidVersion
try:
va, vb = Version(a), Version(b)
except InvalidVersion:
# fallback or handle non-PEP440 case
... -
Alternative (preserve original LooseVersion semantics for heterogeneous version schemes)
pip install looseversion
from looseversion import LooseVersion
if LooseVersion(a) < LooseVersion(b):
...
Notes:
- packaging follows PEP 440 and will raise packaging.version.InvalidVersion for non‑conforming strings; use packaging.parse(...) which returns a LegacyVersion for non‑PEP440 but behavior differs from distutils.LooseVersion. (packaging.pypa.io)
- distutils was deprecated/removed from stdlib; projects migrated by using packaging or the looseversion PyPI package. If CARLA leaderboard compares package versions from PyPI or other PEP‑440 sources, use packaging. If the leaderboard parses arbitrary vendor strings, prefer looseversion. (peps.python.org)
If you paste the specific CARLA leaderboard file(s) that import/use LooseVersion I’ll produce the exact patch.
Citations:
- 1: https://packaging.pypa.io/en/stable/version.html?utm_source=openai
- 2: https://packaging.pypa.io/en/stable/version.html?utm_source=openai
- 3: https://peps.python.org/pep-0632/?utm_source=openai
Replace deprecated distutils.version.LooseVersion removed in Python 3.12.
The distutils module was deprecated in Python 3.10 and removed from the standard library in Python 3.12. Since you're comparing CARLA package versions (which follow PEP 440), use packaging.version.Version instead.
Update the import:
-from distutils.version import LooseVersion
+from packaging.version import VersionUpdate the comparison at line 93:
- if LooseVersion(dist.version) < LooseVersion("0.9.10"):
+ if Version(dist.version) < Version("0.9.10"):Action required: Add packaging to your project dependencies (e.g., code/requirements.txt or the appropriate setup configuration).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/test/run_test.py around line 19, the code imports deprecated
distutils.version.LooseVersion; replace this with packaging.version.Version by
importing Version from packaging.version and update any version comparisons to
construct Version(...) for the CARLA package strings; also add packaging to
project dependencies (e.g., code/requirements.txt or setup config) so the
package is available at runtime.
| client = carla.Client(args.host, args.port) | ||
| if args.timeout: | ||
| client_timeout = args.timeout | ||
| client.set_timeout(client_timeout) |
There was a problem hiding this comment.
Bug: client_timeout may be undefined when args.timeout is falsy.
If args.timeout evaluates to falsy (e.g., 0, None, or empty), line 195 is skipped, and line 196 uses an undefined local variable client_timeout, causing a NameError.
Apply this diff to use the class attribute as the default:
def _setup_simulation(self, args):
"""
Prepares the simulation by getting the client, and setting up the world and
traffic manager settings
"""
client = carla.Client(args.host, args.port)
+ client_timeout = self.client_timeout # Use class default
if args.timeout:
client_timeout = args.timeout
client.set_timeout(client_timeout)📝 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.
| client = carla.Client(args.host, args.port) | |
| if args.timeout: | |
| client_timeout = args.timeout | |
| client.set_timeout(client_timeout) | |
| client = carla.Client(args.host, args.port) | |
| client_timeout = self.client_timeout # Use class default | |
| if args.timeout: | |
| client_timeout = args.timeout | |
| client.set_timeout(client_timeout) |
🤖 Prompt for AI Agents
In code/test/run_test.py around lines 193 to 196, client_timeout may be left
undefined when args.timeout is falsy; set client_timeout to args.timeout or fall
back to the client's default timeout attribute before calling set_timeout.
Concretely, assign client_timeout = args.timeout or client.timeout (or the
equivalent property/method on the client that exposes the default) so
set_timeout always receives a defined value.
| route_records = test_evaluator.statistics_manager._results.checkpoint.records | ||
| flags = [] | ||
| for i, route_record in enumerate(route_records): | ||
| flag = False | ||
| print(Fore.BLUE + f"Test: {TEST_THRESHOLD_AND_NAME[i][1]}") | ||
| print(Fore.CYAN + "TIME TEST:") | ||
| if route_record.meta["duration_game"] > TEST_THRESHOLD_AND_NAME[i][2]: | ||
| print(f"should be in {TEST_THRESHOLD_AND_NAME[i][2]}") | ||
| print(f"needed: {route_record.meta['duration_game']}") | ||
| print(Fore.RED + "FAIL") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
| print(Fore.CYAN + "Collision Test") | ||
| print(Fore.CYAN + "Vehicle") | ||
| if len(route_record.infractions["collisions_vehicle"]) > 0: | ||
| print(route_record.infractions["collisions_vehicle"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| print(Fore.CYAN + "Pedestrian") | ||
| if len(route_record.infractions["collisions_pedestrian"]) > 0: | ||
| print(route_record.infractions["collisions_pedestrian"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| print(Fore.CYAN + "emergency vehicle:") | ||
| if len(route_record.infractions["yield_emergency_vehicle_infractions"]) > 0: | ||
| print(route_record.infractions["yield_emergency_vehicle_infractions"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| print(Fore.CYAN + "other collisions:") | ||
| if len(route_record.infractions["collisions_layout"]) > 0: | ||
| print(route_record.infractions["collisions_layout"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| print(Fore.CYAN + "Running Red light:") | ||
| if len(route_record.infractions["red_light"]) > 0: | ||
| print(route_record.infractions["red_light"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| print(Fore.CYAN + "Running Stop sign:") | ||
| if len(route_record.infractions["stop_infraction"]) > 0: | ||
| print(route_record.infractions["stop_infraction"]) | ||
| print(Fore.RED + "FAILED") | ||
| flag = True | ||
| else: | ||
| print(Fore.GREEN + "SUCCEED") | ||
|
|
||
| flags.append(flag) | ||
| print("check following checkpoint(s)") | ||
| for i, flag in enumerate(flags): | ||
| if flag: | ||
| print(TEST_THRESHOLD_AND_NAME[i][1]) |
There was a problem hiding this comment.
Potential IndexError if route count doesn't match TEST_THRESHOLD_AND_NAME length.
The loop at line 582 iterates over route_records and indexes into TEST_THRESHOLD_AND_NAME[i] without verifying the lengths match. If the number of executed routes differs from the 11 entries in TEST_THRESHOLD_AND_NAME, this will raise an IndexError.
Add bounds checking before the loop:
route_records = test_evaluator.statistics_manager._results.checkpoint.records
flags = []
+if len(route_records) != len(TEST_THRESHOLD_AND_NAME):
+ print(Fore.YELLOW + f"Warning: Route count ({len(route_records)}) doesn't match test config ({len(TEST_THRESHOLD_AND_NAME)})")
for i, route_record in enumerate(route_records):
+ if i >= len(TEST_THRESHOLD_AND_NAME):
+ print(Fore.YELLOW + f"Skipping route {i}: no test configuration available")
+ break
flag = FalseAlternatively, consider using zip() for safer paired iteration:
for route_record, (_, test_name, threshold) in zip(route_records, TEST_THRESHOLD_AND_NAME):🤖 Prompt for AI Agents
In code/test/run_test.py around lines 580 to 646, the loop indexes into
TEST_THRESHOLD_AND_NAME using i while iterating route_records which can raise
IndexError if their lengths differ; replace the indexed loop with a safe paired
iteration (e.g., iterate using zip(route_records, TEST_THRESHOLD_AND_NAME) to
unpack route_record, (id, test_name, threshold)) and use test_name and threshold
variables instead of TEST_THRESHOLD_AND_NAME[i], and when reporting failing
checkpoints iterate over the same zipped pairs (or keep a parallel list of
test_names created from the zip) so flags and names remain aligned and no
out-of-range indexing occurs.
1 participant
Description
Fixes # (868 - Route issues at the beginning of autotest Routes)
Type of change
Does this PR introduce a breaking change?
fixes the existing autotest routes
Most important changes
Checklist:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.