add env loading #649

stan-dot · 2024-09-25T10:07:16Z

No description provided.

stan-dot · 2024-10-07T11:11:11Z

slightly unclear where should the default values come from

#495 (comment)

at the moment the ConfigLoader just starts with all is none and gets updated through the file first

stan-dot · 2024-10-07T11:13:23Z

this needs to be changed because at the moment the tests are failing because of the config loading error@

https://github.com/DiamondLightSource/blueapi/actions/runs/11071605310/job/30763855823?pr=649

stan-dot · 2024-10-07T11:23:13Z

regarding the run sleep plan test, the client._config.port and host give localhost:8000, so the loading is not correct

stan-dot · 2024-10-07T11:27:24Z

harder to test, as the debugger does not pick up the client changes. they do not propagate correctly to the client object from the CLI, though config is called

stan-dot · 2024-10-07T11:30:35Z

OTOH the stomp-loading error is bizarre, and the tkagg is nowhere to be found

File "/workspaces/blueapi/tests/unit_tests/test_cli.py", line 170, in test_valid_stomp_config_for_listener
    assert result.output == dedent("""\
AssertionError: assert 'Backend tkagg is interactive backend. Turning interactive mode on.\n' == 'Subscribing to all bluesky events from localhost:61613\nPress enter to exit\n'
  
  + Backend tkagg is interactive backend. Turning interactive mode on.
  - Subscribing to all bluesky events from localhost:61613
  - Press enter to exit

stan-dot · 2024-10-07T11:38:12Z

for the third of the failing tests, maybe the config is not loaded fully before the assertion is called?

let's check if the cli:main line for use_values_from_yaml is called when debugging the test

stan-dot · 2024-10-07T11:44:05Z

it must be the cli OR env values that override (incorrectly) the prior stomp config

stan-dot · 2024-10-07T11:45:25Z

not quite, it's not overriding, it's just that the use_values does not succeed in mutating the ConfigLoader state

stan-dot · 2024-10-07T11:46:09Z

I forgot the assignment after making a pure function:

        self._values = recursively_updated_map(self._values, values)

stan-dot · 2024-10-07T11:52:36Z

ok actually the last test might be failing due to other kinds of config being invalid, not the scratch field:

Type: string_type, Location: stomp -> host, Message: Input should be a valid string, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/string_type
Type: int_type, Location: stomp -> port, Message: Input should be a valid integer, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/int_type
Type: enum, Location: env -> sources -> 0 -> kind, Message: Input should be 'planFunctions', 'deviceFunctions' or 'dodal', Input: None, Documentation: https://errors.pydantic.dev/2.9/v/enum
Type: path_type, Location: env -> sources -> 0 -> module -> lax-or-strict[lax=union[json-or-python[json=function-after[path_validator(), str],python=is-instance[Path]],function-after[path_validator(), str]],strict=json-or-python[json=function-after[path_validator(), str],python=is-instance[Path]]], Message: Input is not a valid path for <class 'pathlib.Path'>, Input: None
Type: string_type, Location: env -> sources -> 0 -> module -> str, Message: Input should be a valid string, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/string_type
Type: bool_type, Location: env -> events -> broadcast_status_events, Message: Input should be a valid boolean, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/bool_type
Type: literal_error, Location: logging -> level, Message: Input should be 'NOTSET', 'DEBUG', 'INFO', 'WARNING', 'ERROR' or 'CRITICAL', Input: None, Documentation: https://errors.pydantic.dev/2.9/v/literal_error
Type: string_type, Location: api -> host, Message: Input should be a valid string, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/string_type
Type: int_type, Location: api -> port, Message: Input should be a valid integer, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/int_type
Type: string_type, Location: api -> protocol, Message: Input should be a valid string, Input: None, Documentation: https://errors.pydantic.dev/2.9/v/string_type

stan-dot · 2024-10-07T11:55:20Z

thus it comes back to the question of where does the default config come from - which is - the config.py hardcoded values, such as:

class RestConfig(BlueapiBaseModel):
    host: str = "localhost"
    port: int = 8000
    protocol: str = "http"

@stan-dot

#649 @stan-dot For the environment to work, it needed to be in dictionary from so a string to dictionary conversion is needed. This was me playing with it. You probably already working on it, the cli_value are all none unless you assign them so pydantic keep complaining.

Relm-Arrowny · 2024-10-07T14:43:41Z

thus it comes back to the question of where does the default config come from - which is - the config.py hardcoded values, such as:
class RestConfig(BlueapiBaseModel):
    host: str = "localhost"
    port: int = 8000
    protocol: str = "http"

Not sure, if this is the answer you looking for but these are default value inside uvicorn so if you leave it blank when you do run these will be the default value so I guess we just bring it up to so it changeable.

stan-dot · 2024-10-07T14:45:42Z

thus it comes back to the question of where does the default config come from - which is - the config.py hardcoded values, such as:
class RestConfig(BlueapiBaseModel):
    host: str = "localhost"
    port: int = 8000
    protocol: str = "http"
Not sure, if this is the answer you looking for but these are default value inside uvicorn so if you leave it blank when you do run these will be the default value so I guess we just bring it up to so it changeable.

Cool, I didn't realize the uvicorn connection, thanks

codecov · 2024-10-07T15:22:32Z

Codecov Report

Attention: Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.76%. Comparing base (cd91f0f) to head (dc9577c).

Files with missing lines	Patch %	Lines
src/blueapi/config.py	95.55%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   92.62%   92.76%   +0.14%     
==========================================
  Files          35       35              
  Lines        1654     1700      +46     
==========================================
+ Hits         1532     1577      +45     
- Misses        122      123       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stan-dot · 2024-10-08T15:38:21Z

yay it works!

src/blueapi/cli/cli.py

src/blueapi/config.py

src/blueapi/cli/cli.py

tests/unit_tests/test_cli.py

stan-dot · 2024-10-09T12:29:03Z

Thanks @callumforrester for the review, I'll implement shortly

tpoliaw

Definitely needs docs on how to use this. I can't see a way of passing options to the CLI that aren't blocked as unrecognised by click. It would also be good to have a list of options that are expected or available

src/blueapi/cli/cli.py

tpoliaw · 2024-10-09T13:05:47Z

src/blueapi/config.py

+        for key, value in os.environ.items():
+            if key.startswith(prefix):
+                # Convert key to a config path-like structure
+                config_key = key[len(prefix) :].lower()


Suggested change

config_key = key[len(prefix) :].lower()

config_key = key.removeprefix(prefix).lower()

Should this also use the same method as the CLI parsing? eg BLUEAPI_CONFIG_FOO=bar sets values['config']['foo'] = "bar" instead of relying on eval (which presumably requires something like BLUEAPI.CONFIG='{"foo": "bar"}')?

tests/unit_tests/test_config.py

src/blueapi/cli/cli.py

src/blueapi/config.py

tpoliaw · 2024-10-09T13:42:47Z

src/blueapi/config.py

            raise InvalidConfigError(
-                "Something is wrong with the configuration file: \n"
+                f"""Something is wrong with the configuration file:
+                {pretty_error_messages}"""


Does this end up with the first error being weirdly indented?

tpoliaw · 2024-10-09T13:44:17Z

tests/unit_tests/test_config.py

+    new = {"a": {"y": 20, "z": 30}, "c": 4}
+    expected = {"a": {"x": 1, "y": 20, "z": 30}, "b": 3, "c": 4}
+    result = recursively_updated_map(old, new)
+    assert result == expected, f"Expected {expected}, but got {result}"


Should this also test that old wasn't modified?

@stan-dot

#649 @stan-dot For the environment to work, it needed to be in dictionary from so a string to dictionary conversion is needed. This was me playing with it. You probably already working on it, the cli_value are all none unless you assign them so pydantic keep complaining.

@stan-dot

#649 @stan-dot For the environment to work, it needed to be in dictionary from so a string to dictionary conversion is needed. This was me playing with it. You probably already working on it, the cli_value are all none unless you assign them so pydantic keep complaining.

nice, I haven't thought of this lambda use Co-authored-by: Peter Holloway <[email protected]>

stan-dot · 2024-10-31T09:04:52Z

actually there should be a package for that

stan-dot · 2024-10-31T09:07:15Z

@DominicOram please take a look at the examples for this https://github.com/PasaOpasen/py-env-parser?tab=readme-ov-file#examples

if it's not right, the logic is just 200 lines of code so we could copy paste into this repo and adjust should we want to modulate the API - but arguably this would be useful across DLS

Relm-Arrowny · 2024-10-31T10:26:24Z

@DominicOram please take a look at the examples for this https://github.com/PasaOpasen/py-env-parser?tab=readme-ov-file#examples

if it's not right, the logic is just 200 lines of code so we could copy paste into this repo and adjust should we want to modulate the API - but arguably this would be useful across DLS

Not sure if this help or not, I think pydantic has something call setting management that allow you to pick up env variable if it is missing. and you can just do something like

BaseSettings().dict()

to get the dictionary out. But of cause you will have to already have a pydantic baseModel for the data you want to parse.

callumforrester · 2024-10-31T10:31:55Z

@Relm-Arrowny it's worth looking into but I'm not sure how well it works for nested data

stan-dot · 2024-10-31T15:03:15Z

@callumforrester it promises a pretty good level of support. https://docs.pydantic.dev/latest/concepts/pydantic_settings/#nested-model-default-partial-updates

thanks @Relm-Arrowny for finding this, as a feature of pydantic it looks more solid than the library I found

DominicOram · 2024-10-31T17:33:31Z

@DominicOram please take a look at the examples for this https://github.com/PasaOpasen/py-env-parser?tab=readme-ov-file#examples

if it's not right, the logic is just 200 lines of code so we could copy paste into this repo and adjust should we want to modulate the API - but arguably this would be useful across DLS

As just a user of BlueAPI I don't have strong or well-informed opinions about implementation details and I feel like any opinion I do give will just muddy the water. So I defer to @callumforrester and @tpoliaw as the maintainers of BlueAPI to make any decisions around this issue.

stan-dot · 2024-11-04T10:24:39Z

closing as the implementation with pydantic will be entirely different

https://docs.pydantic.dev/latest/concepts/pydantic_settings/#parsing-environment-variable-values

stan-dot added enhancement New feature or request cli Relates to CLI code labels Sep 25, 2024

stan-dot requested review from keithralphs, tpoliaw, callumforrester and DiamondJoseph September 25, 2024 10:07

stan-dot self-assigned this Sep 25, 2024

stan-dot linked an issue Sep 25, 2024 that may be closed by this pull request

Set configuration options via environment variables #495

Open

Relm-Arrowny mentioned this pull request Oct 7, 2024

convert value to dictionary #659

Merged

stan-dot marked this pull request as ready for review October 7, 2024 16:15

callumforrester requested changes Oct 9, 2024

View reviewed changes

tpoliaw reviewed Oct 9, 2024

View reviewed changes

stan-dot force-pushed the 495-set-configuration-options-via-environment-variables branch from d28ee9a to 6213805 Compare October 28, 2024 11:40

stan-dot and others added 18 commits October 29, 2024 15:12

add env loading

02d0532

fix import

f72f8ed

partial fix to the tests

277ba73

investigating the errors

d3f0241

merge

8e10c5d

fix the cli values loading

594da00

add tests for coverage

79875bd

fix some tests and imports

7e365f0

simplify the test to pass

0024033

revert patch change

5028e92

right name used for the patch now

054230a

respond to small feedback, no tests yet

f7bc448

more tests added, partial response to extant feedback

7fc1bb6

Update src/blueapi/config.py

e3c2053

nice, I haven't thought of this lambda use Co-authored-by: Peter Holloway <[email protected]>

respond to feedback

db7f1d1

ruff

817d4d0

fix tests

138a096

stan-dot force-pushed the 495-set-configuration-options-via-environment-variables branch from 6213805 to 138a096 Compare October 29, 2024 15:12

Relm-Arrowny mentioned this pull request Oct 29, 2024

Include convenience bash script for running rabbit mq locally #668

Merged

stan-dot closed this Nov 4, 2024

stan-dot mentioned this pull request Nov 5, 2024

Refactor Config to use Pydantic BaseSettings #699

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add env loading #649

add env loading #649

stan-dot commented Sep 25, 2024

stan-dot commented Oct 7, 2024 •

edited

Loading

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

Relm-Arrowny commented Oct 7, 2024

stan-dot commented Oct 7, 2024

codecov bot commented Oct 7, 2024 •

edited

Loading

stan-dot commented Oct 8, 2024

stan-dot commented Oct 9, 2024

tpoliaw left a comment

tpoliaw Oct 9, 2024

tpoliaw Oct 9, 2024

tpoliaw Oct 9, 2024

stan-dot commented Oct 31, 2024

stan-dot commented Oct 31, 2024 •

edited

Loading

Relm-Arrowny commented Oct 31, 2024 •

edited

Loading

callumforrester commented Oct 31, 2024

stan-dot commented Oct 31, 2024

DominicOram commented Oct 31, 2024

stan-dot commented Nov 4, 2024

	config_key = key[len(prefix) :].lower()
	config_key = key.removeprefix(prefix).lower()

add env loading #649

add env loading #649

Conversation

stan-dot commented Sep 25, 2024

stan-dot commented Oct 7, 2024 • edited Loading

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

stan-dot commented Oct 7, 2024

Relm-Arrowny commented Oct 7, 2024

stan-dot commented Oct 7, 2024

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

stan-dot commented Oct 8, 2024

stan-dot commented Oct 9, 2024

tpoliaw left a comment

Choose a reason for hiding this comment

tpoliaw Oct 9, 2024

Choose a reason for hiding this comment

tpoliaw Oct 9, 2024

Choose a reason for hiding this comment

tpoliaw Oct 9, 2024

Choose a reason for hiding this comment

stan-dot commented Oct 31, 2024

stan-dot commented Oct 31, 2024 • edited Loading

Relm-Arrowny commented Oct 31, 2024 • edited Loading

callumforrester commented Oct 31, 2024

stan-dot commented Oct 31, 2024

DominicOram commented Oct 31, 2024

stan-dot commented Nov 4, 2024

stan-dot commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

stan-dot commented Oct 31, 2024 •

edited

Loading

Relm-Arrowny commented Oct 31, 2024 •

edited

Loading