8000 [Bugfix] Fix logic in setting settings using env vars. by psobolewskiPhD · Pull Request #6669 · napari/napari · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Bugfix] Fix logic in setting settings using env vars. #6669

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 24, 2024
Merged

[Bugfix] Fix logic in setting settings using env vars. #6669

merged 10 commits into from
Jul 24, 2024

Conversation

psobolewskiPhD
Copy link
Member
@psobolewskiPhD psobolewskiPhD commented Feb 18, 2024 •
edited
Loading

References and relevant issues

Closes: #6637

Description

This PR fixes the logic in the nested_env_settings method to:

  1. ensure that for each sub-field the env vars are set, but nesting all of the subfield related checks and setting within the subfield for loop
  2. and also makes sure to not break loop too early, such that more than one env var could be set.

I added tests that fail on main, plus a sub-cases that passes on main but wasn't tested.

Edit: berg, the diff is annoying.
The first change is just indenting all the logic under for subf in field_type.__fields__.values():
The 2nd change is removing the early break in the else of for e in subf.field_info.extra.get('env_names', []):. This break in the for loop else was breaking the parent loop early.

@github-actions github-actions bot added tests Something related to our tests topic:preferences Issues relating to the creation of new preference fields/panels labels Feb 18, 2024
@psobolewskiPhD
Copy link
Member Author

I think this PR is the minimum to fix the bugged behavior which has annoyed me for some time.
However, I still have outstanding questions regarding inconsistencies I discovered:

  1. Should NAPARI_THEME=light napari work? that field has env="napari_theme" but because appearance is EventedModel and not EventedSettings it doesn't do anything.
    it looks like it was considered in the original implementation, but no action was taken before merge:
    Refactor settings manager to allow setting all preferences with env vars and context managers #2932 (comment)
    tests use the full name though. Option is to either remove the non-functioning env="napari_theme" or switch appearance to EventedSettings to enable it to work.

  2. For the other EventedSettings (plugins and experimental), do we want to set a proper prefix so the raw names like RDP_EPSILON=1 napari don't work?

  3. Currently, no plugin fields have env set, so should it just be converted to use EventedModal?

For some more context, see my investigation here:
#6637 (comment)

@psobolewskiPhD psobolewskiPhD added the bugfix PR with bugfix label Feb 18, 2024
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Feb 18, 2024
@codecov Codecov
Copy link
codecov bot commented Feb 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.66%. Comparing base (2a0cee2) to head (d39c7af).

Files Patch % Lines
napari/settings/_base.py 73.33% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6669      +/-   ##
==========================================
- Coverage   92.73%   92.66%   -0.07%     
==========================================
  Files         610      610              
  Lines       55382    55404      +22     
==========================================
- Hits        51357    51341      -16     
- Misses       4025     4063      +38

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

Czaki
Czaki approved these changes Feb 23, 2024
View reviewed changes
Copy link
Collaborator
@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have downloaded it and it works. I think that I understand and agree with patches, but I think that in general code should be refactored and splitted on more functions.

But I'm ok with merge it.

So @psobolewskiPhD if you do not have time to refactor code, the add ready to merge label.

@psobolewskiPhD
Copy link
Member Author

I suspect that the handling could be largely replaced with pydantic stuffs, because that now supports this kind of stuff, which it didn't when this code was written. However, I might be missing some nuance and I hesitate to volunteer for this job 😬
I mean it could be a fun project...

Any thoughts on my questions #6669 (comment)

@jni jni modified the milestones: 0.5.0, 0.5.1 Jul 8, 2024
@jni
Copy link
Member
jni commented Jul 8, 2024

This looks close to ready so I'm marking it for the spiffy new 0.5.1 milestone. Code refactors can come later.

@Czaki Czaki merged commit 444e8db into napari:main Jul 24, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Czaki Czaki Czaki approved these changes

Assignees
No one assigned
Labels
bugfix PR with bugfix tests Something related to our tests topic:preferences Issues relating to the creation of new preference fields/panels
Projects
None yet
Milestone
0.5.1
Development

Successfully merging this pull request may close these issues.

environmental vars are ignored if an experimental setting is set (e.g. async is enabled)
3 participants
@psobolewskiPhD @jni @Czaki
0