Conversation
| setattr(cls, name, None) | ||
|
|
||
| def __init__(self, *args, **kwargs) -> None: # type: ignore[no-untyped-def] | ||
| def __init__(self, global_config: GlobalConfig = global_config, **kwargs: Any): |
There was a problem hiding this comment.
As an alternative to asking subclasses (that need a custom __init__()) to include global_config here, I could move it into the ModuleConfig itself? Then subclasses will just pass kwargs alone.
Greptile SummaryThis PR standardizes module configuration by migrating from dataclasses to Pydantic models and enforcing a fixed Key Changes:
Issues Found:
Impact: Confidence Score: 3/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class BaseConfig {
<<Pydantic BaseModel>>
+model_config: arbitrary_types_allowed
}
class ModuleConfig {
+rpc_transport: RPCSpec
+tf_transport: TFSpec
+frame_id_prefix: str | None
+frame_id: str | None
}
class NativeModuleConfig {
+executable: str
+build_command: str | None
+extra_args: list
+extra_env: dict
+compute_cli_args() list
}
class Configurable~ConfigT~ {
<<Generic>>
+default_config: type[ConfigT]
+config: ConfigT
+__init__(**kwargs)
}
class ModuleBase~ModuleConfigT~ {
+default_config: ModuleConfig
-_rpc: RPCSpec | None
-_tf: TFSpec | None
+__init__(config_args: dict, global_config: GlobalConfig)
}
class Module~ModuleConfigT~ {
+__init__(global_config: GlobalConfig, **kwargs)
+set_ref(ref)
+start()
+stop()
}
class NativeModule~NativeConfig~ {
+default_config: NativeModuleConfig
-_process: Popen | None
+__init__(global_config: GlobalConfig, **kwargs)
}
BaseConfig <|-- ModuleConfig : inherits
ModuleConfig <|-- NativeModuleConfig : inherits
Configurable <|-- ModuleBase : inherits
ModuleBase <|-- Module : inherits
Module <|-- NativeModule : inherits
Configurable --> BaseConfig : uses ConfigT
ModuleBase --> ModuleConfig : uses ModuleConfigT
note for ModuleBase "Fixed __init__ signature:\nconfig_args + global_config"
note for Module "Standardized signature:\nglobal_config + **kwargs"
note for BaseConfig "Runtime validation\nvia Pydantic"
Last reviewed commit: 28a43f7 |
dimos/perception/experimental/temporal_memory/temporal_memory.py
Outdated
Show resolved
Hide resolved
|
The tests are still failing.
|
| # TODO: All of these should be fixed, but it's easier commit autofixes first | ||
| "A001", "A002", "B008", "B017", "B019", "B024", "B026", "B904", "C901", "E402", "E501", "E721", "E722", "E741", "F811", "F821", "F821", "F821", "N801", "N802", "N803", "N806", "N817", "N999", "RUF003", "RUF009", "RUF012", "RUF034", "RUF043", "RUF059", "UP007", | ||
| # This breaks runtime type checking (both for us, and users introspecting our APIs) | ||
| "TC001", "TC002", "TC003" |
There was a problem hiding this comment.
I fixed this recently. The linter needs to know which classes need to have their annotations at run time. Are there more classes besides Module ones now?
[tool.ruff.lint.flake8-type-checking]
runtime-evaluated-base-classes = ["dimos.core.module.Module"]
There was a problem hiding this comment.
I guess it's mainly ModuleConfig now. But, that may reference other classes which are not subclassed from ModuleConfig, creating a lot of extra false positives. I still get test failures after adding ModuleConfig and letting ruff fix things.
This rule is one of my pet peeves, as it achieves nothing (all these modules are getting imported at runtime somewhere, so there's going to be no noticeable improvement to import time), and thus only causing problems and making code slightly less readable. As noted in the comment, there's a small chance users may want to introspect our APIs at runtime and this would break any attempts to do that.
| class ModuleConfig(BaseConfig): | ||
| rpc_transport: type[RPCSpec] = LCMRPC | ||
| tf_transport: type[TFSpec] = LCMTF | ||
| tf_transport: type[TFSpec] = LCMTF # type: ignore[type-arg] |
There was a problem hiding this comment.
Are you sure # type: ignore[type-arg] is needed? It wasn't before.
There was a problem hiding this comment.
Yeah, pydantic won't let me resolve it:
Subscripting `type[]` with an already prametrized type is not supported.
It's because I made it generic, so mypy wants it subscripted.
|
hey for the future for easier reviews, usually we ask for oneliners to test, but this is an API change so could you show us the new API with example code blocks, like what does this change mean for us building modules.. either this or docs update link we can read |
|
Also do we need to accept global_config as arg even if not using global config in our module? why not pass kwargs directly? |
I think that goes back to my open question above: If we move it to the ModuleConfig, then subclasses don't need to worry about defining the parameter, and it's then always there if needed. Most modules won't need to define a custom |
This reverts commit 790397c.
* Adjust config definitions for future enhancements * Fixes * Update module.py * Update module.py * Update blueprints.py * Fixes * Fixes * Fixes * Apply suggestion from @Dreamsorcerer * Update person_follow.py * Update person_follow.py * Fixes * Fixes * Fixes * Fixes --------- Co-authored-by: Sam Bull <Sam.B@snowfalltravel.com>
This reverts commit 790397c.
* Adjust config definitions for future enhancements * Fixes * Update module.py * Update module.py * Update blueprints.py * Fixes * Fixes * Fixes * Apply suggestion from @Dreamsorcerer * Update person_follow.py * Update person_follow.py * Fixes * Fixes * Fixes * Fixes --------- Co-authored-by: Sam Bull <Sam.B@snowfalltravel.com>
This reverts commit 790397c.
Problem
Working towards #1082, this helps standardise setting in modules which can then be expanded upon to enable further things such as CLI arguments to be injected. They are also now type checked at runtime via Pydantic.
Solution
Primary change to developers is that the
__init__()method now uses a fixed signature and all configurable options appear in the config. That config is now a pydantic model, so it gets verified when the module is instantiated.A lot of typing fixes were also made by changing incorrect references to Module to ModuleBase and similar adjustments.
Breaking Changes
Subclasses of Module must now match the
__init__()signature of the Module class.