feat(flight): KML trajectory export endpoint#73
Conversation
GET /flights/{id}/kml returns a KML file of the flight trajectory.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds support for exporting a flight’s trajectory as a Google Earth KML download via a new GET /flights/{id}/kml endpoint, integrating the route/controller/service layers and covering the new behavior with unit tests.
Changes:
- Added
GET /flights/{flight_id}/kmlroute returning a KML file download response. - Implemented controller + service support for generating KML bytes using RocketPy’s
FlightDataExporter. - Added route unit tests for success, 404, and 500 cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/unit/test_routes/test_flights_route.py |
Adds unit tests validating KML endpoint response bytes, headers, and error propagation. |
src/services/flight.py |
Implements KML export in FlightService using FlightDataExporter and a temporary file. |
src/routes/flight.py |
Adds the /flights/{flight_id}/kml endpoint returning a KML file download response. |
src/controllers/flight.py |
Exposes get_flight_kml() controller method with standard exception handling wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n tests pylint W0613 (unused-argument) on 5 schema-validation tests failed the build step (pylint exit 4), blocking this PR and the stacked KML PR #73. These tests only assert a 422 from schema validation, so they never touch the controller; and mock_controller_instance is @pytest.fixture(autouse=True), so it still runs for every test regardless — the parameter was vestigial. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n tests pylint W0613 (unused-argument) on 5 schema-validation tests failed the build step (pylint exit 4), blocking this PR and the stacked KML PR #73. These tests only assert a 422 from schema validation, so they never touch the controller; and mock_controller_instance is @pytest.fixture(autouse=True), so it still runs for every test regardless — the parameter was vestigial. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI unblocked ✅ (same pylint fix) — stacked on #72Cherry-picked the same one-line CI fix as #72 (this branch is stacked on Why this matters for Jarvis: the results screen defaults to a Cesium/KML 3D trajectory hero and calls Merge order: #72 first (this is stacked on it), then this one, then deploy. — pushed as part of a Claude Code audit of the beta (Gui). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_routes/test_flights_route.py (1)
543-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the service and controller export path.
These tests mock
controller.get_flight_kml, so they cannot catch failures inFlightService.get_flight_kml()export, byte reading, cleanup, orFlightController.get_flight_kml()wiring. Add focused tests that mockFlightDataExporter.export_kmland verify returned bytes plus cleanup on success and exporter failure.🤖 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 `@tests/unit/test_routes/test_flights_route.py` around lines 543 - 572, Add focused unit tests for FlightService.get_flight_kml and FlightController.get_flight_kml that mock FlightDataExporter.export_kml rather than only controller.get_flight_kml. Cover successful byte reading and returned content, exporter failure propagation, and cleanup in both success and failure paths, while verifying the controller forwards the request correctly.
🤖 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 `@src/controllers/flight.py`:
- Around line 166-168: Update the KML generation flow in the controller method
around FlightService.from_flight_model and get_flight_kml so the synchronous
export runs via asyncio.to_thread or an equivalent bounded worker-pool
mechanism, keeping the event loop non-blocking. Ensure the worker uses the
dedicated FlightService/FlightDataExporter instance for that flight and preserve
the returned KML result.
---
Nitpick comments:
In `@tests/unit/test_routes/test_flights_route.py`:
- Around line 543-572: Add focused unit tests for FlightService.get_flight_kml
and FlightController.get_flight_kml that mock FlightDataExporter.export_kml
rather than only controller.get_flight_kml. Cover successful byte reading and
returned content, exporter failure propagation, and cleanup in both success and
failure paths, while verifying the controller forwards the request correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e299f2b-3e10-4d16-a50b-beea42e1148b
📒 Files selected for processing (5)
src/controllers/flight.pysrc/routes/flight.pysrc/services/flight.pytests/unit/test_routes/test_flights_route.pytests/unit/test_routes/test_motors_route.py
| flight = await self.get_flight_by_id(flight_id) | ||
| flight_service = FlightService.from_flight_model(flight.flight) | ||
| return flight_service.get_flight_kml() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)src/controllers/flight\.py$|src/\w+/service.*rocket|FlightService|flight_service|flight\.py|api\.py' || true
echo "== outline controller =="
ast-grep outline src/controllers/flight.py --view expanded || true
echo "== relevant controller lines =="
sed -n '130,190p' src/controllers/flight.py
echo "== search FlightService and KML calls =="
rg -n "class FlightService|from_flight_model|get_flight_kml|get_flight_kml\(|RocketPy|FlightService" src tests || trueRepository: RocketPy-Team/Infinity-API
Length of output: 13835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FlightService relevant implementation =="
sed -n '47,100p' src/services/flight.py
sed -n '500,575p' src/services/flight.py
echo "== routes controller call =="
sed -n '300,335p' src/routes/flight.py
echo "== dependency pins around rocketpy and async worker packages =="
sed -n '1,220p' pyproject.toml
sed -n '1,220p' requirements.txt 2>/dev/null || true
sed -n '1,220p' requirements.dev.txt 2>/dev/null || true fd -a 'pyproject.toml|requirements.*|poetry.lock|Pipfile.lock' . | sed -n '1,80p'
echo "== inspect installed rocketpy Flight APIs if available =="
python3 - <<'PY'
import importlib.util, inspect
spec = importlib.util.find_spec("rocketpy.simulation.flight")
if spec is None:
print("rocketpy.simulation.flight not importable in sandbox")
else:
from rocketpy.simulation.flight import Flight as RocketPyFlight
print("RocketPyFlight exported")
for name in dir(RocketPyFlight):
if "kml" in name.lower() or name == "show_kml":
print(name)
if hasattr(RocketPyFlight, "show_kml"):
print(inspect.signature(RocketPyFlight.show_kml))
PYRepository: RocketPy-Team/Infinity-API
Length of output: 7661
🌐 Web query:
RocketPy Python FlightDataExporter export_kml source Thread safe
💡 Result:
In RocketPy, the FlightDataExporter class is the recommended way to export simulation data, including KML files for trajectory visualization [1][2]. The export_kml method is accessed by instantiating FlightDataExporter with a Flight object and calling the method with the desired parameters, such as file_name and altitude_mode [3][1]. Regarding thread safety, the FlightDataExporter.export_kml method is not explicitly documented as thread-safe. RocketPy's general architecture, particularly its core simulation classes, is not designed with inherent thread safety for concurrent access to the same object instance [4][5]. While the MonteCarlo simulation engine supports parallel execution, it achieves this by distributing independent Flight instances across separate processes using Python's multiprocessing module rather than by sharing state across threads [4][5]. If you need to perform concurrent exports, it is safer to ensure that each thread or process operates on its own dedicated FlightDataExporter instance and corresponding Flight object to avoid race conditions or data corruption [4][5].
Citations:
- 1: https://github.com/RocketPy-Team/RocketPy/blob/master/docs/user/first_simulation.rst
- 2: https://docs.rocketpy.org/en/latest/user/first_simulation.html
- 3: https://github.com/RocketPy-Team/RocketPy/blob/cb15a393/rocketpy/simulation/flight_data_exporter.py
- 4: https://deepwiki.com/RocketPy-Team/RocketPy/8-stochastic-analysis-and-monte-carlo
- 5: https://deepwiki.com/RocketPy-Team/RocketPy/8.2-montecarlo-simulation-engine
Offload KML generation from the event loop.
get_flight_kml() holds the FastAPI event loop while constructing a RocketPy Flight, running the synchronous FlightDataExporter.export_kml() path, and reading/writing temp files. Run the export in a bounded worker pool, such as asyncio.to_thread, so concurrent downloads don’t block unrelated requests. If export state is shared per FlightDataExporter, ensure the export operates on the dedicated exporter/flight instance in that worker.
🤖 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 `@src/controllers/flight.py` around lines 166 - 168, Update the KML generation
flow in the controller method around FlightService.from_flight_model and
get_flight_kml so the synchronous export runs via asyncio.to_thread or an
equivalent bounded worker-pool mechanism, keeping the event loop non-blocking.
Ensure the worker uses the dedicated FlightService/FlightDataExporter instance
for that flight and preserve the returned KML result.
There was a problem hiding this comment.
not sure, my memory on the event loop is lacking, but IIRC FastAPI already handles everything async anyway , it will just wrap the sync work in a thread inside the pool and let it compete in scheduling with everything else, can you confirm @coderabbitai ?
There was a problem hiding this comment.
🧩 Analysis chain
🤖 web_search executed:
FastAPI documentation async def synchronous code called inside async path operation function threadpool Starlette run_in_threadpool
Length of output: 20825
@GabrielBarberini, confirmed by the FastAPI docs: it automatically runs a path operation declared with ordinary def in an external thread pool, but it does not automatically offload synchronous utility calls made from an async def handler.
Here, get_flight_kml() is async def, so the direct synchronous call:
return flight_service.get_flight_kml()runs on the event-loop thread and blocks it until RocketPy finishes exporting and the temporary-file I/O completes. Explicitly awaiting asyncio.to_thread(...) or fastapi.concurrency.run_in_threadpool(...) is the appropriate fix.
The docs also explicitly note that this automatic thread-pool behavior applies to synchronous path operations and dependencies—not arbitrary synchronous functions invoked within an async path operation: https://fastapi.tiangolo.com/async/.
There was a problem hiding this comment.
@coderabbitai what would you suggest then ? should we declare this route in specific as sync (def) or keep with it async ? what would you do in case we keep it async to solve that problem?
There was a problem hiding this comment.
@GabrielBarberini, I’d keep the route and controller async. They already await database work (get_flight_by_id), so changing this endpoint to def would require awkwardly bridging back into async code and provides no benefit.
Offload only the blocking RocketPy export call to FastAPI/Starlette’s managed thread pool:
from starlette.concurrency import run_in_threadpool`@controller_exception_handler`
async def get_flight_kml(
self,
flight_id: str,
) -> bytes:
flight = await self.get_flight_by_id(flight_id)
flight_service = FlightService.from_flight_model(flight.flight)
return await run_in_threadpool(flight_service.get_flight_kml)This keeps database access on the async path while moving the synchronous RocketPy processing and temporary-file I/O off the event-loop thread. The FlightService is local to this request, so the worker operates on a dedicated service/exporter instance.
asyncio.to_thread(flight_service.get_flight_kml) would also work, but I’d prefer run_in_threadpool(...) in this FastAPI application because it uses Starlette/AnyIO’s managed worker-pool behavior.
There was a problem hiding this comment.
@aasitvora99 , @Gui-FernandesBR , I think run_in_threadpool(flight_service.get_flight_kml) seems to be a clean solution, wdyt?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
GET /flights/{id}/kml returns a KML file of the flight trajectory.
Summary by CodeRabbit
New Features
Bug Fixes
Tests