Conversation
| ) | ||
|
|
||
| return resp, flight_id | ||
| if resp.flight_plan_status in { |
There was a problem hiding this comment.
nit: improve readability with early return with return resp, flight_id, None if status is not among those
(could also apply to inner if with return resp, flight_id, flight_info)
There was a problem hiding this comment.
Sounds good -- done
|
|
||
|
|
||
| def values_exactly_equal( | ||
| as_requested: str | Number | StringBasedDateTime, |
There was a problem hiding this comment.
How were those types chosen? Is it because that's the exhaustive list of types the JSON values can take? Although I expect not because there is no object-like type.
Also, nit: define a type alias for those? Notably if we expect this list will be extended in the future.
There was a problem hiding this comment.
These are just the types this CompatibilityEvaluator (values_exactly_equal) is capable of evaluating. They do correspond to leaf JSON values (list and object/dict are inner branches of the JSON value tree). Technically, both parameters could be type-annotated as Any since that matches the general CompatibilityEvaluator pattern, but listing the specific types acts as an additional form of documentation regarding when this CompatibilityEvaluator can be used.
I think in this case, a type alias would just add an unnecessary level of indirection and additional content since it's only used twice on adjacent lines.
| return | ||
| if dtype == StringBasedDateTime: | ||
| assert isinstance(as_planned, StringBasedDateTime) | ||
| assert isinstance(as_requested, StringBasedDateTime) |
There was a problem hiding this comment.
Redundant since we are in if isinstance(as_requested, dtype) and if dtype == StringBasedDateTime blocks.
There was a problem hiding this comment.
With sufficiently sophisticated logic this is redundant, but alas, basedpyright's logic is not that sophisticated and it produces an error if this assertion of type is removed. It would probably be ideal for it to be a little smarter, but I don't think I mind a few extra assertions like this in exchange for the validation it performs, and usually it's pretty smart.
| ) | ||
| return | ||
| if dtype == StringBasedDateTime: | ||
| assert isinstance(as_planned, StringBasedDateTime) |
There was a problem hiding this comment.
Isn't this redundant as well since we previously checked for if not isinstance(as_planned, dtype)?
There was a problem hiding this comment.
Similar to the other line, this is needed for basedpyright
|
|
||
|
|
||
| def _resolve_addresses( | ||
| paths: Iterable[JSONPath] | JSONPath, content: dict[str, Any] |
There was a problem hiding this comment.
Since we check below if that's an str type?
| paths: Iterable[JSONPath] | JSONPath, content: dict[str, Any] | |
| paths: Iterable[JSONPath] | JSONPath | str, content: dict[str, Any] |
There was a problem hiding this comment.
JSONPath is just a str with extra semantic meaning -- checking for str below is nearly equivalent to checking for JSONPath, so I've just changed that isinstance check to JSONPath rather than str.
| details=f"As requested, {address} has {len(as_requested)} elements, but as planned, {address} has {len(as_planned)} elements (equality expected)", | ||
| ) | ||
| return | ||
| for i, v in enumerate(as_requested): |
There was a problem hiding this comment.
This implies the order is important. Is it actually? Wouldn't two lists with the same items but in different order be equivalent? I don't remember some entities in the APIs we use where it is. But I have not checked that exhaustively.
There was a problem hiding this comment.
In general, order is important -- for instance, this might be a list of points describing the outline of an area. I'm sure there are applications where order isn't important, but I think it is important by default.
| self._begin_step_fragment() | ||
| oi_ref = self._operational_intent_shared_check(flight_info, skip_if_not_found) | ||
| if flight_info is None: | ||
| raise ScenarioLogicError( |
There was a problem hiding this comment.
Isn't ScenarioDidNotStopError more appropriate here?
There was a problem hiding this comment.
The reason this isn't a ScenarioDidNotStopError is that that error requires a check to be specified (longer name probably something like "Scenario didn't stop due to a failed check that I expected to stop the scenario") and that check is no longer available here. ScenarioLogicError is the more generic error that doesn't required a check.
| flight1_planned, | ||
| ) | ||
| # TODO(#1326): Validate that flight as planned still allows this scenario to proceed | ||
| assert as_planned is not None |
There was a problem hiding this comment.
Are the asserts in this file (and also in others) really required? If there is an actual probability of this happening shouldn't it be handled properly? (i.e. if because of USS -> fail, if because of inconsistency in test suite -> raise exception)
There was a problem hiding this comment.
The asserts are almost exclusively (perhaps wholly exclusively) for the benefit of basedpyright. They are effectively providing a human-machine interface layer where the human writes something they think is obviously true (via an assertion), and then the machine validates behavior given that assumption. A human reviewer can review the explicit assumptions the human author is making (making sure the assertions are justified), and then those assumptions are programmatically checked at runtime too. So, while I don't know that they're enormously valuable, I do think they have some value since they enable basedpyright's more extensive validation and also cause things to fail fast as soon as some assumption is violated.
| ImplicitDict.parse(query.response.json["as_planned"], FlightPlan) | ||
| ) | ||
| except ValueError: | ||
| # Best effort failed so it's ok to ignore additional `as_planned` field |
There was a problem hiding this comment.
Would it make sense to log as a warning the error here? That would probably help troubleshoot issues, if any.
There was a problem hiding this comment.
Makes sense; added warning.
| if "as_planned" in inject_resp and inject_resp.as_planned: | ||
| resp.as_planned = inject_resp.as_planned.to_flight_plan() | ||
| for k, v in inject_resp.items(): | ||
| if k not in {"planning_result", "flight_plan_status", "has_conflict"}: |
There was a problem hiding this comment.
To be sure that it is not a mistake: has_conflict is no longer required here?
There was a problem hiding this comment.
Yes, that was my intent -- I believe "has_conflict" should actually have not been there previously. The purpose of this list is to avoid overwriting already-populated fields in the response when transferring any additional metadata/extra fields that may have been provided. Since has_conflict wasn't already populated and there's no reason to specifically exclude it, I think it should be removed.
2 participants
This PR addresses (but does not fully resolve) #1326 by:
as_plannedfield in flight_planning responses after "snapping" requested start times to the current wall time (preventing users from rewriting history by saying a flight started in the past)as_plannedin the flight_planning API response and populate a newas_plannedfield in the business object responseas_plannedresponse matched what was requested (very strictly, for now) via a new injection fidelity checkTo accomplish 4 above, the large injection_evaluation.py tool is added. This attempts to make evaluation of as-planned flights as future-scalable as practical by performing bulk field-by-field comparisons automatically according to comparators defined per JSONPath in addition to a default comparator.
Ideally, each scenario would define what is acceptable for an as-planned flight (though hopefully they would reuse many of the same definitions -- e.g., "anything that overlaps the other flight"). However, making all these changes in one PR would be far too large. Instead, placeholder TODOs referencing the appropriate issue are added in the places where each scenario could (in the future) define what is acceptable for as-planned flights, and one "global" validation is added in flight_planning/test_steps.py:submit_flight which requires all fields in as_planned to match exactly (status quo), except start times are allowed to be adjusted forward to the wall clock.
In addition to the standard CI, I also tested as_planned validation by temporarily setting mock_uss to set start time to wall clock + 5 seconds (instead of just wall clock in this actual PR) and observing that the Injection fidelity check failed. See the diff of mock_uss/flights/planning.py on the second commit for this approach (though the timing did not correspond to exactly commit 2).
A number of small errors/basedpyright findings are resolved as discovered during development of the core feature.