-
Notifications
You must be signed in to change notification settings - Fork 7
Make Settings Pydantic and use the power of BaseSettings to simplify CLI #700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
98b5dce
7cd6dea
5004584
c3ed3f8
775574e
2c436d3
bd4151e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,14 +11,15 @@ | |||||||||||
| from bluesky_stomp.models import Broker | ||||||||||||
| from observability_utils.tracing import setup_tracing | ||||||||||||
| from pydantic import ValidationError | ||||||||||||
| from pydantic_settings.sources import PathType | ||||||||||||
| from requests.exceptions import ConnectionError | ||||||||||||
|
|
||||||||||||
| from blueapi import __version__ | ||||||||||||
| from blueapi.cli.format import OutputFormat | ||||||||||||
| from blueapi.client.client import BlueapiClient | ||||||||||||
| from blueapi.client.event_bus import AnyEvent, BlueskyStreamingError, EventBusClient | ||||||||||||
| from blueapi.client.rest import BlueskyRemoteControlError | ||||||||||||
| from blueapi.config import ApplicationConfig, ConfigLoader | ||||||||||||
| from blueapi.config import ApplicationConfig | ||||||||||||
| from blueapi.core import OTLP_EXPORT_ENABLED, DataEvent | ||||||||||||
| from blueapi.worker import ProgressEvent, Task, WorkerEvent | ||||||||||||
|
|
||||||||||||
|
|
@@ -32,24 +33,17 @@ | |||||||||||
| "-c", "--config", type=Path, help="Path to configuration YAML file", multiple=True | ||||||||||||
| ) | ||||||||||||
| @click.pass_context | ||||||||||||
| def main(ctx: click.Context, config: Path | None | tuple[Path, ...]) -> None: | ||||||||||||
| # if no command is supplied, run with the options passed | ||||||||||||
|
|
||||||||||||
| config_loader = ConfigLoader(ApplicationConfig) | ||||||||||||
| if config is not None: | ||||||||||||
| configs = (config,) if isinstance(config, Path) else config | ||||||||||||
| for path in configs: | ||||||||||||
| if path.exists(): | ||||||||||||
| config_loader.use_values_from_yaml(path) | ||||||||||||
| else: | ||||||||||||
| raise FileNotFoundError(f"Cannot find file: {path}") | ||||||||||||
|
|
||||||||||||
| ctx.ensure_object(dict) | ||||||||||||
| loaded_config: ApplicationConfig = config_loader.load() | ||||||||||||
|
|
||||||||||||
| ctx.obj["config"] = loaded_config | ||||||||||||
| def main(ctx: click.Context, config: PathType) -> None: | ||||||||||||
| # Override default yaml_file path in the model_config if `config` is provided | ||||||||||||
| ApplicationConfig.model_config["yaml_file"] = config | ||||||||||||
| app_config = ApplicationConfig() # Instantiates with customized sources | ||||||||||||
|
Comment on lines
+58
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should: I'm concerned this. mutation of global state is an antipattern. Could you instead subclass the config?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's how Pydantic uses it and this is not regular state but settings also this is not Java
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutation of global state is an antipattern in any programming language (good thread). Now I have to remember to address this if I use ApplicationConfig.model_config["yaml_file"] = NoneThere is nothing that makes this obvious to me as a developer.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the mutable settings option is here:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like what we're trying to do was just not a well-envisioned use case: pydantic/pydantic-settings#346 Should maybe reconsider the use of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is true, turns out that pydantic-settings isn't as well-finished product like pydantic itself |
||||||||||||
| ctx.obj["config"] = app_config | ||||||||||||
|
|
||||||||||||
| # note: this is the key result of the 'main' function, it loaded the config | ||||||||||||
| # and due to 'pass context' flag above | ||||||||||||
| # it's left for the handler of words that are later in the stdin | ||||||||||||
| logging.basicConfig( | ||||||||||||
| format="%(asctime)s - %(message)s", level=loaded_config.logging.level | ||||||||||||
| format="%(asctime)s - %(message)s", level=app_config.logging.level | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| if ctx.invoked_subcommand is None: | ||||||||||||
|
|
@@ -173,6 +167,7 @@ def listen_to_events(obj: dict) -> None: | |||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| fmt = obj["fmt"] | ||||||||||||
|
|
||||||||||||
| def on_event( | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # config_manager.py | ||
|
|
||
| from blueapi.config import ApplicationConfig | ||
|
|
||
|
|
||
| class ConfigManager: | ||
| """Manages application configuration in a way that’s easy to test and mock.""" | ||
|
|
||
| _config: ApplicationConfig | ||
|
|
||
| def __init__(self, config: ApplicationConfig = None): | ||
| if config is None: | ||
| ApplicationConfig.model_config["yaml_file"] = None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should: Again, I think you should find an alternative to mutating global state, if you fix the instance in CLI then you shouldn't need this line. |
||
| config = ApplicationConfig() | ||
| self._config = config | ||
|
|
||
| def get_config(self) -> ApplicationConfig: | ||
| """Retrieve the current configuration.""" | ||
| return self._config | ||
|
|
||
| def set_config(self, new_config: ApplicationConfig): | ||
| """ | ||
| This is a setter function that the main process uses | ||
| to pass the config into the subprocess | ||
| """ | ||
| self._config = new_config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,20 +9,18 @@ | |
| from blueapi.config import ApplicationConfig, StompConfig | ||
| from blueapi.core.context import BlueskyContext | ||
| from blueapi.core.event import EventStream | ||
| from blueapi.service.config_manager import ConfigManager | ||
| from blueapi.service.model import DeviceModel, PlanModel, WorkerTask | ||
| from blueapi.worker.event import TaskStatusEnum, WorkerState | ||
| from blueapi.worker.task import Task | ||
| from blueapi.worker.task_worker import TaskWorker, TrackableTask | ||
|
|
||
| """This module provides interface between web application and underlying Bluesky | ||
| context and worker""" | ||
| context and worker | ||
|
|
||
| """ | ||
|
|
||
| _CONFIG: ApplicationConfig = ApplicationConfig() | ||
|
|
||
|
|
||
| def config() -> ApplicationConfig: | ||
| return _CONFIG | ||
| config_manager = ConfigManager() | ||
|
|
||
|
|
||
| def set_config(new_config: ApplicationConfig): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could: Can we remove this now? |
||
|
|
@@ -34,16 +32,16 @@ def set_config(new_config: ApplicationConfig): | |
| @cache | ||
| def context() -> BlueskyContext: | ||
| ctx = BlueskyContext() | ||
| ctx.with_config(config().env) | ||
| env_config = config_manager.get_config().env | ||
| ctx.with_config(env_config) | ||
| return ctx | ||
|
|
||
|
|
||
| @cache | ||
| def worker() -> TaskWorker: | ||
| worker = TaskWorker( | ||
| context(), | ||
| broadcast_statuses=config().env.events.broadcast_status_events, | ||
| ) | ||
| env_config = config_manager.get_config().env | ||
| should_broadcast_status_events: bool = env_config.events.broadcast_status_events | ||
| worker = TaskWorker(context(), broadcast_statuses=should_broadcast_status_events) | ||
| worker.start() | ||
| return worker | ||
|
|
||
|
|
@@ -79,7 +77,7 @@ def stomp_client() -> StompClient | None: | |
| def setup(config: ApplicationConfig) -> None: | ||
| """Creates and starts a worker with supplied config""" | ||
|
|
||
| set_config(config) | ||
| config_manager.set_config(config) | ||
|
|
||
| # Eagerly initialize worker and messaging connection | ||
|
|
||
|
|
||
This file was deleted.

Uh oh!
There was an error while loading. Please reload this page.