Change simulation encoding workaround to official rocketpy encoders #51
Conversation
GabrielBarberini
requested review from
Copilot
and removed request for
aasitvora99
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces generic attribute extraction with a configurable encoder for RocketPy objects, introduces DiscretizeConfig and rocketpy_encoder, updates services to use the encoder, adapts view models to accept dynamic encoded attributes, and configures serialization to exclude None fields for API models and views. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service (e.g., RocketService)
participant Obj as RocketPy Object
participant Config as DiscretizeConfig
participant Encoder as rocketpy_encoder
participant View as SimulationView
Service->>Obj: read instance (self.<obj>)
Service->>Config: DiscretizeConfig.for_<type>()
Service->>Encoder: rocketpy_encoder(Obj, Config)
Encoder->>Obj: discretize callable attributes, serialize
Encoder-->>Service: encoded dict
Service->>View: SimulationView(**encoded dict)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GabrielBarberini
requested review from
aasitvora99,
Gui-FernandesBR and
phmbressan
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the simulation encoding approach by replacing the deprecated get_instance_attributes utility with official RocketPy encoders and introducing a new DiscretizeConfig class for better dynamic attribute handling. The changes enable more flexible and extensible simulation data serialization across all RocketPy object types.
Key Changes:
- Replaced
get_instance_attributeswith newrocketpy_encoderfunction using official RocketPyRocketPyEncoder - Added
DiscretizeConfigclass for configurable function discretization parameters - Updated all simulation view classes to use Pydantic's
ConfigDictwithextra='allow'for dynamic attributes
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.py | Added DiscretizeConfig class and rocketpy_encoder function replacing deprecated encoding approach |
| src/views/*.py | Updated simulation view classes to use ConfigDict and replaced specific attribute types with Any |
| src/services/*.py | Replaced get_instance_attributes calls with rocketpy_encoder using appropriate configs |
| src/repositories/*.py | Added exclude_none=True to model_dump() calls for cleaner serialization |
| src/models/interface.py | Added ser_json_exclude_none=True to base model configuration |
|
|
||
| if callable(attr_value) and isinstance(attr_value, Function): | ||
| try: | ||
| discretized_func = Function(attr_value.source) |
There was a problem hiding this comment.
Creating a new Function from attr_value.source may not preserve all properties of the original Function object. Consider using copy.deepcopy(attr_value) or a proper Function copy method if available.
| discretized_func = Function(attr_value.source) | |
| discretized_func = copy.deepcopy(attr_value) |
Copilot uses AI. Check for mistakes.
|
|
||
|
|
||
| class MotorSimulation(BaseModel): | ||
| class MotorSimulation(ApiBaseView): |
There was a problem hiding this comment.
Changing the base class from BaseModel to ApiBaseView is a breaking change that could affect existing API consumers who depend on the original inheritance structure.
| class MotorSimulation(ApiBaseView): | |
| from pydantic import BaseModel | |
| class MotorSimulation(BaseModel): |
Copilot uses AI. Check for mistakes.
|
|
||
| # Core Rocket attributes (always present) | ||
| radius: Optional[float] = None | ||
| mass: Optional[float] = None |
There was a problem hiding this comment.
The removed attribute area is not present in the new core attributes list. If this attribute is still needed, it should be included or documented why it was removed.
Copilot uses AI. Check for mistakes.
| time: Optional[Any] = None | ||
| solution: Optional[Any] = None | ||
|
|
||
| # Function attributes (discretized by rocketpy_encoder, serialized by RocketPyEncoder) |
There was a problem hiding this comment.
This comment appears to be a duplicate of the comment on line 33. The distinction between 'Key Flight Function attributes' and 'Function attributes' is unclear.
| # Function attributes (discretized by rocketpy_encoder, serialized by RocketPyEncoder) | |
| # Additional Flight Function attributes: Supplementary outputs that may be useful for advanced analysis or diagnostics (also discretized by rocketpy_encoder, serialized by RocketPyEncoder) |
Copilot uses AI. Check for mistakes.
| include_outputs=True, | ||
| include_function_data=True, | ||
| ) | ||
| return json.loads(json_str) |
There was a problem hiding this comment.
The encoding process involves creating a JSON string and then immediately parsing it back to a dictionary. Consider using the encoder directly to return a dictionary without the JSON round-trip if possible.
| return json.loads(json_str) | |
| encoder = RocketPyEncoder( | |
| include_outputs=True, | |
| include_function_data=True, | |
| ) | |
| return encoder.default(obj_copy) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/models/interface.py(1 hunks)src/repositories/flight.py(1 hunks)src/repositories/motor.py(1 hunks)src/repositories/rocket.py(1 hunks)src/views/interface.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-22T21:53:21.139Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#44
File: src/models/interface.py:12-17
Timestamp: 2025-02-22T21:53:21.139Z
Learning: In Pydantic v2, both bare dict and ConfigDict can be used for model_config, but ConfigDict is recommended for better type hints and IDE support.
Applied to files:
src/views/interface.py
📚 Learning: 2025-02-22T21:53:21.139Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#44
File: src/models/interface.py:12-17
Timestamp: 2025-02-22T21:53:21.139Z
Learning: Pydantic v2 requires using ConfigDict for model configuration. Plain dictionaries lack type safety, proper validation, and full feature support.
Applied to files:
src/views/interface.py
🧬 Code Graph Analysis (3)
src/repositories/rocket.py (4)
src/repositories/interface.py (2)
insert(225-232)update_by_id(235-239)src/models/rocket.py (1)
RocketModel(13-63)src/routes/rocket.py (1)
update_rocket(56-68)tests/unit/test_routes/test_rockets_route.py (1)
test_update_rocket(357-365)
src/repositories/flight.py (2)
src/repositories/interface.py (2)
insert(225-232)update_by_id(235-239)src/models/flight.py (2)
FlightModel(7-71)DELETED(53-54)
src/repositories/motor.py (5)
src/repositories/interface.py (3)
insert(225-232)find_by_id(242-249)update_by_id(235-239)src/models/motor.py (2)
MotorModel(16-95)UPDATED(73-74)tests/unit/test_routes/test_motors_route.py (2)
test_update_motor_invalid_input(301-305)test_update_motor(291-298)src/routes/motor.py (1)
update_motor(56-68)tests/unit/test_routes/conftest.py (1)
stub_motor_dump(19-30)
🔇 Additional comments (6)
src/repositories/rocket.py (2)
22-22: Insert payload: LGTMUsing model_dump(exclude_none=True) on create avoids persisting nulls for optional fields and aligns with the new global serialization strategy.
30-32: Re-verify null-field clearing behavior in update_by_idI ran the suggested search for any tests sending
None/nullin update payloads and found no matches. However, absence of tests isn’t evidence that no clients rely on clearing fields vianull. Please confirm whether any API consumers or tests expect setting a field tonullto clear it. If so, address one of the following:
- Keep
exclude_none=Trueand update your API docs/tests to note thatnullcan no longer clear fields.- Restore clearing behavior by splitting the payload into
$setand$unset(example below).File: src/repositories/rocket.py
Lines: 30–32- await self.update_by_id( - rocket.model_dump(exclude_none=True), data_id=rocket_id - ) + raw = rocket.model_dump() + to_unset = {k: "" for k, v in raw.items() if v is None} + to_set = {k: v for k, v in raw.items() if v is not None} + await self.update_by_id( + {"$set": to_set, "$unset": to_unset}, data_id=rocket_id + )src/repositories/flight.py (1)
22-22: Insert payload: consistent and correctexclude_none=True on create is consistent with the new model/view serialization. Good.
src/repositories/motor.py (1)
22-22: Create flow: LGTMexclude_none=True reduces stored document size and matches the PR-wide policy.
src/views/interface.py (1)
1-1: No JSON null expectations in view responses — approving use of ConfigDictThe only
Noneassertion found (tests/unit/test_repositories/test_repository_interface.py:235) pertains to the repository stub’s return value, not to JSON payloads. There are no tests checking for": null"in view output, so omittingnullfields viaser_json_exclude_none=Trueis safe.src/models/interface.py (1)
24-24: BaseModel JSON now omits None globallyThe
ser_json_exclude_none=Trueflag on your BaseModel (src/models/interface.py:24) means that anyNone‐valued fields will be dropped from the JSON output (model_dump_json()/.json()), matching patterns in repos/views.
- I ran a search for any code or tests that inspect explicit
null/Nonevalues in JSON serialization and found none.- Existing calls using
exclude_none=Trueon.dict()remain unaffected.Action required:
- Update your API or developer documentation to note that fields with
Nonevalues will no longer appear in serialized JSON (clients expecting explicitnullentries should be informed).fix_required
| await self.update_by_id( | ||
| flight.model_dump(exclude_none=True), data_id=flight_id | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update semantics changed: None no longer clears fields
Same concern as in RocketRepository: exclude_none=True will ignore fields explicitly set to None, preventing clearing. Decide whether:
- This is intended and document it, or
- Implement $unset handling for None fields.
Happy to draft an interface-level change to support $unset cleanly if desired.
🏁 Script executed:
#!/bin/bash
# Sanity check: ensure all repositories applied the same exclude_none=True policy
rg -n "model_dump\(exclude_none=True\)" src/repositoriesLength of output: 650
Support clearing fields set to None via $unset
Currently, all repository update calls use model_dump(exclude_none=True), so any fields explicitly set to None are omitted and won’t be cleared in the database. Please decide on one of the following:
• If omitting None is the intended behavior, add clear documentation (both inline and in our API docs) stating that fields cannot be cleared via None.
• Otherwise, extend update_by_id to handle $unset:
– Build a $set payload from non-None values
– Build an $unset payload from keys with None values
– Send both in a single update operation
Apply the chosen approach consistently in:
- src/repositories/rocket.py (update_by_id call at line 31)
- src/repositories/motor.py (update_by_id call at line 31)
- src/repositories/flight.py (update_by_id call at line 31)
Let me know if you’d like a draft for the interface-level refactor to support $unset.
| await self.update_by_id( | ||
| motor.model_dump(exclude_none=True), data_id=motor_id | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
PUT behavior change: explicit nulls ignored on update
Applying exclude_none=True here prevents clients from nulling fields. If unsetting is a supported use case, consider the $set/$unset approach mentioned for Rocket/Flight repositories; otherwise, update API documentation to make this explicit.
🏁 Script executed:
#!/bin/bash
# Look for tests asserting null-able fields after update
rg -n "update_.*motor|client\.put\('/motors" -A 8 tests | rg -n "None|null"Length of output: 179
Explicit nulls are ignored on updates; choose to implement unset logic or update docs accordingly
- Location:
src/repositories/motor.py, around theupdate_by_id( motor.model_dump(exclude_none=True), … )call. exclude_none=Truedrops any fields clients explicitly set tonull, so you can’t use this endpoint to unset a value.- If unsetting fields is a required use case:
- Adopt a
$set/$unsetpattern (see Rocket/Flight repos) to translateNoneinto an$unsetoperation.
- Adopt a
- Otherwise, update the API documentation to clearly state that
nullvalues are ignored and cannot be used to clear existing fields.
🤖 Prompt for AI Agents
In src/repositories/motor.py around lines 30 to 32, the call to update_by_id
uses motor.model_dump(exclude_none=True) which strips fields explicitly set to
None so clients cannot unset values; either implement unset logic or document
behavior. To fix, implement a $set/$unset translation: serialize the model
without excluding None, iterate keys and build an update payload that places
non-None fields into a $set dict and None-valued fields into a $unset dict (or
omit $unset if unset not supported), then call update_by_id with the composed
Mongo-style update object; alternatively, if unsetting is intentionally
unsupported, update the API docs and client contract to state that nulls are
ignored and cannot clear fields.
| PlainSerializer(to_python_primitive), | ||
| ] | ||
| try: | ||
| json_str = json.dumps( |
There was a problem hiding this comment.
@phmbressan @GabrielBarberini not sure who this is for but when i run the env class simulation I get an incorrect date format here
"date": [2025, 6, 9, 23]
get_instance_attributes parses it as a natural datetime object. Is this a encoder issue? Observed this behavior while printing this json_str variable.
Pull Request Overview
This PR shifts the simulation encoding approach to utilize the official RocketPy encoders and a new DiscretizeConfig for improved dynamic attribute support across simulation models. Key changes include:
Summary by CodeRabbit
Refactor
Chores
Documentation