core: services: Move from pykson to pydantic #3615
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR replaces pykson-based settings models and managers across services with pydantic-based implementations, removes all pykson artifacts and dependencies, updates migration and serialization logic accordingly, and adds comprehensive tests for the FirmwareUploader to boost coverage. Entity relationship diagram for updated settings data structureserDiagram
SETTINGSV1 {
int VERSION
DefaultSettings default
string[] blacklist
Interface[] interfaces
ServiceTypes[] advertisement_types
}
DefaultSettings {
string[] domain_names
string[] advertise
string ip
}
Interface {
string name
string[] domain_names
string[] advertise
string ip
}
ServiceTypes {
string name
string protocol
int port
string properties
}
SETTINGSV1 ||--o| DefaultSettings : has
SETTINGSV1 ||--|{ Interface : has
SETTINGSV1 ||--|{ ServiceTypes : has
Interface ||--o| DefaultSettings : has
EXTENSIONS {
string identifier
string name
string docker
string tag
string permissions
bool enabled
string user_permissions
}
MANIFESTS {
string identifier
bool enabled
int priority
bool factory
string name
string url
}
KRAKENSETTINGSV1 {
int VERSION
EXTENSIONS[] extensions
}
KRAKENSETTINGSV2 {
int VERSION
MANIFESTS[] manifests
}
KRAKENSETTINGSV2 ||--|{ MANIFESTS : has
KRAKENSETTINGSV1 ||--|{ EXTENSIONS : has
BRIDGESETTINGSV1 {
int VERSION
BridgeSettingsSpecV1[] specs
}
BridgeSettingsSpecV1 {
string serial_path
int baudrate
string ip
int udp_port
}
BRIDGESETTINGSV1 ||--|{ BridgeSettingsSpecV1 : has
BRIDGESETTINGSV2 {
int VERSION
BridgeSettingsSpecV2[] specsv2
}
BridgeSettingsSpecV2 {
int udp_target_port
int udp_listen_port
string serial_path
int baudrate
string ip
}
BRIDGESETTINGSV2 ||--|{ BridgeSettingsSpecV2 : has
NMEAINJECTORSETTINGSV1 {
int VERSION
NmeaInjectorSettingsSpecV1[] specs
}
NmeaInjectorSettingsSpecV1 {
string kind
int port
int component_id
}
NMEAINJECTORSETTINGSV1 ||--|{ NmeaInjectorSettingsSpecV1 : has
PINGSETTINGSV1 {
int VERSION
Ping1dSettingsSpecV1[] ping1d_specs
}
Ping1dSettingsSpecV1 {
string port
bool mavlink_enabled
}
PINGSETTINGSV1 ||--|{ Ping1dSettingsSpecV1 : has
WIFISETTINGSV1 {
int VERSION
bool hotspot_enabled
string hotspot_ssid
string hotspot_password
bool smart_hotspot_enabled
}
Class diagram for updated settings models (pykson → pydantic)classDiagram
class DefaultSettings {
+List[str] domain_names
+List[str] advertise
+str ip
}
class Interface {
+str name
+List[str] domain_names
+List[str] advertise
+str ip
+get_phys()
}
class ServiceTypes {
+str name
+Literal["_udp", "_tcp"] protocol
+int port
+Optional[str] properties
+get_properties()
}
class SettingsV1 {
+int VERSION = 1
+DefaultSettings default
+List[str] blacklist
+List[Interface] interfaces
+List[ServiceTypes] advertisement_types
+migrate(data)
+get_interface_or_create_default(name)
}
class SettingsV2 {
+int VERSION = 2
+migrate(data)
}
class SettingsV3 {
+int VERSION = 3
+migrate(data)
}
class SettingsV4 {
+int VERSION = 4
+str vehicle_name
+migrate(data)
}
SettingsV2 --|> SettingsV1
SettingsV3 --|> SettingsV2
SettingsV4 --|> SettingsV3
SettingsV1 o-- DefaultSettings
SettingsV1 o-- Interface
SettingsV1 o-- ServiceTypes
class ExtensionSettings {
+str identifier
+str name
+str docker
+str tag
+str permissions
+bool enabled
+str user_permissions
+settings()
+container_name()
}
class ManifestSettings {
+str identifier
+bool enabled
+int priority
+bool factory
+str name
+str url
}
class KrakenSettingsV1 {
+int VERSION = 1
+Sequence[ExtensionSettings] extensions
+migrate(data)
}
class KrakenSettingsV2 {
+int VERSION = 2
+Sequence[ManifestSettings] manifests
+migrate(data)
}
KrakenSettingsV2 --|> KrakenSettingsV1
KrakenSettingsV1 o-- ExtensionSettings
KrakenSettingsV2 o-- ManifestSettings
class BridgeSettingsSpecV1 {
+str serial_path
+int baudrate
+str ip
+int udp_port
+from_spec(spec)
+__eq__(other)
}
class BridgeSettingsSpecV2 {
+int udp_target_port
+int udp_listen_port
+str serial_path
+int baudrate
+str ip
+from_spec(spec)
+__eq__(other)
}
class BridgetSettingsV1 {
+int VERSION = 1
+List[BridgeSettingsSpecV1] specs
+migrate(data)
}
class BridgetSettingsV2 {
+int VERSION = 2
+List[BridgeSettingsSpecV2] specsv2
+migrate(data)
}
BridgetSettingsV2 --|> BridgetSettingsV1
BridgetSettingsV1 o-- BridgeSettingsSpecV1
BridgetSettingsV2 o-- BridgeSettingsSpecV2
class NmeaInjectorSettingsSpecV1 {
+str kind
+int port
+int component_id
+__eq__(other)
}
class NmeaInjectorSettingsV1 {
+int VERSION = 1
+List[NmeaInjectorSettingsSpecV1] specs
+migrate(data)
}
NmeaInjectorSettingsV1 o-- NmeaInjectorSettingsSpecV1
class Ping1dSettingsSpecV1 {
+str port
+bool mavlink_enabled
+__str__()
+new(port, enabled)
}
class PingSettingsV1 {
+int VERSION = 1
+List[Ping1dSettingsSpecV1] ping1d_specs
+migrate(data)
}
PingSettingsV1 o-- Ping1dSettingsSpecV1
class WifiSettingsV1 {
+int VERSION = 1
+bool hotspot_enabled
+str hotspot_ssid
+str hotspot_password
+bool smart_hotspot_enabled
+migrate(data)
}
Class diagram for updated settings manager usageclassDiagram
class PydanticManager {
+settings
+load()
+save()
+load_from_file()
}
class Beacon {
+PydanticManager manager
+load_default_settings()
}
class Bridget {
+PydanticManager _settings_manager
}
class Kraken {
+PydanticManager _manager
}
class ManifestManager {
+PydanticManager _manager
}
class TrafficController {
+PydanticManager _settings_manager
}
class Ping1DDriver {
+PydanticManager manager
}
class AbstractWifiManager {
+PydanticManager _settings_manager
}
Beacon o-- PydanticManager
Bridget o-- PydanticManager
Kraken o-- PydanticManager
ManifestManager o-- PydanticManager
TrafficController o-- PydanticManager
Ping1DDriver o-- PydanticManager
AbstractWifiManager o-- PydanticManager
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Several Pydantic models use mutable default values (e.g.
List[str] = []or nested model instances) which can lead to shared state—consider usingField(default_factory=list)ordefault_factoryfor nested models instead. - The
_fetch_settingsmethod now always returns a list (even for single matches) but its signature still suggested a single object return in some cases—verify all call sites expect a list or adjust the signature to match actual behavior. - There’s a lot of repeated
PydanticManagerinstantiation across services—consider extracting a factory or base class helper to DRY up that initialization logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several Pydantic models use mutable default values (e.g. `List[str] = []` or nested model instances) which can lead to shared state—consider using `Field(default_factory=list)` or `default_factory` for nested models instead.
- The `_fetch_settings` method now always returns a list (even for single matches) but its signature still suggested a single object return in some cases—verify all call sites expect a list or adjust the signature to match actual behavior.
- There’s a lot of repeated `PydanticManager` instantiation across services—consider extracting a factory or base class helper to DRY up that initialization logic.
## Individual Comments
### Comment 1
<location> `core/services/beacon/settings.py:92` </location>
<code_context>
- self.VERSION = SettingsV1.VERSION
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ default: DefaultSettings = DefaultSettings(domain_names=[], advertise=[], ip="")
+ blacklist: List[str] = []
+ interfaces: List[Interface] = []
</code_context>
<issue_to_address>
**issue (bug_risk):** Using mutable default values for Pydantic fields can lead to shared state across instances.
Directly assigning DefaultSettings as a default value causes all SettingsV1 instances to share the same object, leading to potential side effects if mutated. Use default_factory or a lambda to ensure each instance gets a unique DefaultSettings object.
</issue_to_address>
### Comment 2
<location> `core/services/beacon/settings.py:93-94` </location>
<code_context>
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ default: DefaultSettings = DefaultSettings(domain_names=[], advertise=[], ip="")
+ blacklist: List[str] = []
+ interfaces: List[Interface] = []
+ advertisement_types: List[ServiceTypes] = []
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default arguments for list fields may cause shared state issues.
Use default_factory in Pydantic to ensure each instance has its own list, avoiding unintended shared state.
</issue_to_address>
### Comment 3
<location> `core/services/beacon/settings.py:95` </location>
<code_context>
+ default: DefaultSettings = DefaultSettings(domain_names=[], advertise=[], ip="")
+ blacklist: List[str] = []
+ interfaces: List[Interface] = []
+ advertisement_types: List[ServiceTypes] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for advertisement_types may cause shared state problems.
Use default_factory for advertisement_types to ensure each SettingsV1 instance has its own list.
</issue_to_address>
### Comment 4
<location> `core/services/kraken/settings.py:47` </location>
<code_context>
- self.VERSION = SettingsV1.VERSION
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ extensions: Sequence[ExtensionSettings] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for extensions may cause shared state issues.
Use default_factory in Pydantic to create a new list for each instance and avoid unintended shared state.
</issue_to_address>
### Comment 5
<location> `core/services/kraken/settings.py:61` </location>
<code_context>
-
- self.VERSION = SettingsV2.VERSION
+ VERSION: int = 2
+ manifests: Sequence[ManifestSettings] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for manifests may cause shared state issues.
Use default_factory to ensure each SettingsV2 instance gets its own list, preventing unintended shared state.
</issue_to_address>
### Comment 6
<location> `core/services/bridget/settings.py:30` </location>
<code_context>
- self.VERSION = SettingsV1.VERSION
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ specs: List[BridgeSettingsSpecV1] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for specs may cause shared state issues.
Use default_factory in Pydantic to create a new list for each instance and avoid unintended shared state.
</issue_to_address>
### Comment 7
<location> `core/services/bridget/settings.py:67` </location>
<code_context>
-
- self.VERSION = SettingsV2.VERSION
+ VERSION: int = 2
+ specsv2: List[BridgeSettingsSpecV2] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for specsv2 may cause shared state issues.
Use default_factory to ensure each SettingsV2 instance gets its own list, preventing unintended shared state.
</issue_to_address>
### Comment 8
<location> `core/services/nmea_injector/nmea_injector/settings.py:20` </location>
<code_context>
- self.VERSION = SettingsV1.VERSION
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ specs: List[NmeaInjectorSettingsSpecV1] = []
def migrate(self, data: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for specs may cause shared state issues.
Use default_factory in Pydantic to create a new list for each instance and avoid unintended shared state.
</issue_to_address>
### Comment 9
<location> `core/services/ping/settings.py:21` </location>
<code_context>
- ping1d_specs = pykson.ObjectListField(Ping1dSettingsSpecV1)
+class SettingsV1(PydanticSettings):
+ VERSION: int = 1
+ ping1d_specs: List[Ping1dSettingsSpecV1] = []
# no settings for ping360 as of V1
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutable default for ping1d_specs may cause shared state issues.
Use default_factory in Pydantic to assign a new list for each instance and avoid unintended shared state.
</issue_to_address>
### Comment 10
<location> `core/services/ardupilot_manager/firmware/test_FirmwareUpload.py:70-79` </location>
<code_context>
+ @pytest.mark.asyncio
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for upload failure scenarios in async upload method.
Please add tests for cases where the subprocess fails or exceptions occur during upload to verify error handling.
</issue_to_address>
### Comment 11
<location> `core/services/ardupilot_manager/firmware/test_FirmwareUpload.py:49-54` </location>
<code_context>
+ uploader = FirmwareUploader()
+ uploader.validate_binary()
+
+ def test_validate_binary_failure(self) -> None:
+ with patch("shutil.which", return_value="ardupilot_fw_uploader.py"):
+ uploader = FirmwareUploader()
+
+ with patch("subprocess.check_output", side_effect=subprocess.CalledProcessError(2, "cmd", "error output")):
+ with pytest.raises(InvalidUploadTool, match="Binary returned 2 on '--help' call: error output"):
+ uploader.validate_binary()
+
+ def test_set_autopilot_port(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for validate_binary when the binary is missing.
Testing the scenario where shutil.which returns None will help confirm that validate_binary properly handles missing binaries.
```suggestion
def test_validate_binary_missing(self) -> None:
with patch("shutil.which", return_value=None):
uploader = FirmwareUploader()
with pytest.raises(InvalidUploadTool, match="Could not find ardupilot_fw_uploader.py binary"):
uploader.validate_binary()
def test_set_autopilot_port(self) -> None:
with patch("shutil.which", return_value="ardupilot_fw_uploader.py"):
uploader = FirmwareUploader()
new_port = pathlib.Path("/dev/autopilot2")
uploader.set_autopilot_port(new_port)
assert uploader._autopilot_port == new_port
```
</issue_to_address>
### Comment 12
<location> `core/services/beacon/settings.py:156` </location>
<code_context>
def migrate(self, data: Dict[str, Any]) -> None:
if data["VERSION"] == SettingsV3.VERSION:
return
if data["VERSION"] < SettingsV3.VERSION:
super().migrate(data)
try:
if not any(interface["name"] == "uap0" for interface in data["interfaces"]):
data["interfaces"].append(
Interface(name="uap0", domain_names=["blueos-hotspot"], advertise=["_http"], ip="ips[0]").dict()
)
except Exception as e:
logger.error(f"unable to update SettingsV2 to SettingsV3: {e}")
data["VERSION"] = SettingsV3.VERSION
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(
interface["name"] != "uap0" for interface in data["interfaces"]
):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nicoschmdt
force-pushed
the
move-away-from-pykson
branch
from
0695c52 to
8dd6cd7
Compare
nicoschmdt
force-pushed
the
move-away-from-pykson
branch
from
8dd6cd7 to
0e3fdc3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix: #2015
Had to create more tests to attain +75% of code coverage
Summary by Sourcery
Migrate all core service settings and managers from pykson to pydantic, remove legacy pykson support, and introduce new unit tests for firmware uploading to improve coverage.
Enhancements:
Build:
Tests: