Repository navigation
Add GpfIsochroneLayer tool - #211
LionelZoubritzky-IGN wants to merge 1 commit into
Conversation
| .number() | ||
| .finite() | ||
| .positive() | ||
| .max(NAVIGATION_ISOCHRONE_MAX_TIME_MINUTES) |
There was a problem hiding this comment.
600 minutes is the upstream ceiling, not what the proxy can serve within PROXY_UPSTREAM_TIMEOUT (10 s): measured today from the Orange amphitheater (your test case), car takes 3.0 s at 120 min, 9.5 s at 240, 18.8 s at 360 and 34.2 s at 600 (pedestrian stays around 1 s). Every car request above ~4 h therefore ends in a 504 at map load, which the LLM never sees since it got a valid data_url, so documenting it in config.md isn't enough. I'd cap at 120 like travel_time_filter: 3x margin, and a single limit for the LLM to learn.
| import { NAVIGATION_SOURCE } from "../gpf/navigation.js"; | ||
| import logger from "../logger.js"; | ||
|
|
||
| const GPF_ISOCHRONE_LAYER_TOOL_DESCRIPTION = [ |
There was a problem hiding this comment.
Beyond the missing travel_time_filter cross-reference (both ways: zone vs objects inside it), the description lacks what the LLM needs to choose and fill arguments: coverage (see the schema comment), that only car and pedestrian exist (no bike or public transport: tell the user rather than substitute a mode), that the zone is computed from the departure point, and that lon/lat should come from geocode. The no-proxy error should suggest travel_time_filter as fallback, and the generic sentence in config.md:48 and dev.md:102 ("Sans proxy, utiliser gpf_get_features / gpf_get_feature_by_id") is now false for this tool.
| // Departure point of an isochrone. Flat `lon`/`lat`, exactly like every spatial | ||
| // filter (`intersects_point_filter`, `dwithin_point_filter`, ...), so the LLM sees | ||
| // one point convention across the whole surface. | ||
| const isochronePointSchema = z.object({ |
There was a problem hiding this comment.
The departure point is only checked against world bounds, but bdtopo-valhalla routes on the French network only: Geneva (car, 30 min) returns a 200 zone around Annemasse that does not even contain the departure point, Strasbourg's zone stops at the Rhine, and London or New York end as an opaque 502 at map load. Two cheap fixes: one description line on coverage (French road network only, the zone stops at borders, a foreign point is snapped or fails), and the network-free bbox bound that #191 already adds in b4742e3 (NAVIGATION_BBOX), which belongs here since this PR introduces the shared point schema. minutes also needs a floor such as .min(1): 1e-7 is sent as costValue=1e-7 (upstream 400) and 0.01 gives "No path found".
| function getProxyIsochroneClient(): NavigationIsochroneClient { | ||
| cachedProxyIsochroneClient ??= new NavigationIsochroneClient( | ||
| new RateLimiter({ name: "GPF_NAVIGATION_PROXY", maxCalls: getEnv().GPF_NAVIGATION_PROXY_RATE_LIMIT, period: 1 }), | ||
| (url) => fetchJSONGetWithLimit(url, getEnv().PROXY_UPSTREAM_TIMEOUT * 1000, getEnv().PROXY_MAX_RESPONSE_BYTES, "d'isochrone"), |
There was a problem hiding this comment.
Isochrone maps naturally load several layers at once (rings, two profiles), but the proxy's navigation limiter is a fixed window of 5 starts per second that throws, shared with travel_time layers, and MCP Carto currently loads every layer twice (on tool input and on tool result). So a plain 3-ring pedestrian map (5/10/15 min) already gets a 500 on its 6th request, and Carto then wipes the whole map while the LLM was told the layers are displayed (reproduced against the built proxy). The mechanism predates this PR, but this kind makes it the main use case: I'd let the proxy's navigation limiter wait for the next window (bounded wait, no upstream call replayed, so not a retry), or at least answer 503 with Retry-After; the Cache-Control comment below also helps.
| // Do NOT forward error.serviceDetail (raw upstream WFS text, English, internal | ||
| // Do NOT forward error.serviceDetail (raw upstream WFS or GPF text, English, internal | ||
| // column names) to the client: like the other branches, return a fixed FR | ||
| // message. The upstream detail is logged server-side by the caller. |
There was a problem hiding this comment.
The comment above still says a 4xx means an upstream contract issue because client data already passed validation. That's false for this kind: "No path found" or an out-of-bbox point come from the input itself, yet the proxy answers 502 "réponse inexploitable" (blaming IGN) and logs an error. I'd map navigation 400/404 to a 404 with a fixed French detail (e.g. "Aucune zone calculable depuis ce point."), log it at warn, and keep "Le service WFS" for FeatureCardinalityError, which only the by-id WFS path throws.
2 participants
Description
Add a new
gpf_isochrone_layertool that returns the layer for an isochrone query as an opaque proxy url.Related issues (if applicable)
Close #143
Motivation
Test: "Show all places accessible by walking 20 minutes starting from the Orange amphitheater"
Implementation
Rely on existing navigation and itinerary code (based on GPF's isochrone endpoint).
Minor "TRAVEL_TIME" -> "NAVIGATION" refactor.
Add a new "isochrone" token to the proxy.
Testing
New dedicated tests.
npm run build && npm run verify:fast-> 609 tests all greenTODOs
See subsequent PR of the stack: #212 and #214
Checklist