Skip to content
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

Use ChainMap for UserCommands #375

Merged
merged 10 commits into from
Feb 28, 2024
Merged

Use ChainMap for UserCommands #375

merged 10 commits into from
Feb 28, 2024

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Feb 26, 2024

This makes UserCommands a subclass of astar_utils.NestedChainMap, which is a bang-string-key variant subclass of the standard library's collections.ChainMap. This enables a separation of configuration "layers" into global settings and default, optical train-wide settings and, in the future, effect-level properties and kwargs. Any changes applied through simple assignment (i.e. __setitem__, e.g. cmd["foo"] = 42) will only modify the top layer of this object, thus not influence any other instance building on the same base layer. The individual levels are "by ref", so any change that is applied to the global settings (e.g. logging level) will be seen by all instances. All of this is an advantage over just updating dicts, and potentially making deep copies to avoid cross-contamination.

Additionally, I made some refactoring changes to the rather convoluted .update() method of UserCommands. It's still not quite where I want it to be, but one step at a time. I also added a whole bunch of debug-level logging to, well, debug the YAML loading process, mostly. This could be reduced again in the future, once it's more stable (or once [if] we change the way instrument packages are handled anyway...).

This also bumps the required version of astar-utils to 0.2.2, to include the new bang-string classes from there.

@teutoburg teutoburg added refactor Implementation improvement API How users interact with the software labels Feb 26, 2024
@teutoburg teutoburg self-assigned this Feb 26, 2024
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 79.33884% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 74.21%. Comparing base (d786495) to head (d31c490).

Files Patch % Lines
scopesim/commands/user_commands.py 78.81% 25 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #375      +/-   ##
==============================================
- Coverage       74.29%   74.21%   -0.08%     
==============================================
  Files              56       56              
  Lines            7818     7854      +36     
==============================================
+ Hits             5808     5829      +21     
- Misses           2010     2025      +15     

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

scopesim/defaults.yaml Outdated Show resolved Hide resolved
@teutoburg
Copy link
Contributor Author

Any ideas why the RTD build fails on this @hugobuddel ? I can't really make heads or tails of the error it produces...

@hugobuddel
Copy link
Collaborator

Any ideas why the RTD build fails on this @hugobuddel ? I can't really make heads or tails of the error it produces...

No. I just (tried to) rebuild RTD for the dev_master branch and that also fails. So I think it is caused by a change on their end. Maybe we still need to do something, but it is unrelated to this PR I think.

@teutoburg
Copy link
Contributor Author

3296 Segmentation fault: 11 pip install poetry==1.7.1, lol

@teutoburg
Copy link
Contributor Author

Any ideas why the RTD build fails on this @hugobuddel ? I can't really make heads or tails of the error it produces...

No. I just (tried to) rebuild RTD for the dev_master branch and that also fails. So I think it is caused by a change on their end. Maybe we still need to do something, but it is unrelated to this PR I think.

Oh yeah, I just saw that the dev_master fails with the same cryptic error. So that's nothing to do with this here. Still annoying...

@teutoburg teutoburg added the dependencies Related to or updating any dependencies label Feb 27, 2024
@teutoburg teutoburg marked this pull request as ready for review February 27, 2024 18:09
@teutoburg
Copy link
Contributor Author

I'll go ahead and merge this. We did discuss it, I addressed the previous review comments and tests are passing.

@teutoburg teutoburg merged commit bde4455 into dev_master Feb 28, 2024
24 of 25 checks passed
@teutoburg teutoburg deleted the fh/chainmap branch February 28, 2024 10:32
@hugobuddel
Copy link
Collaborator

I'll go ahead and merge this. We did discuss it, I addressed the previous review comments and tests are passing.

Yeah fine. I didn't do a full review yet, as the PR was still marked draft, but we covered the important points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API How users interact with the software dependencies Related to or updating any dependencies refactor Implementation improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants