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 dynaconf for user preferences #99

Merged
merged 22 commits into from
Sep 13, 2022
Merged

Use dynaconf for user preferences #99

merged 22 commits into from
Sep 13, 2022

Conversation

gselzer
Copy link
Collaborator

@gselzer gselzer commented Sep 6, 2022

This PR introduces dynaconf as a preferences management tool, and removes bugs associated with relative paths.

@gselzer gselzer added bug Something isn't working enhancement New feature or request labels Sep 6, 2022
@gselzer gselzer added this to the 0.1.0 milestone Sep 6, 2022
@gselzer gselzer self-assigned this Sep 6, 2022
@gselzer gselzer linked an issue Sep 6, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2022

Codecov Report

Merging #99 (f901740) into main (99a115c) will decrease coverage by 0.40%.
The diff coverage is 76.73%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   92.30%   91.89%   -0.41%     
==========================================
  Files          39       40       +1     
  Lines        3078     3233     +155     
==========================================
+ Hits         2841     2971     +130     
- Misses        237      262      +25     
Impacted Files Coverage Δ
tests/widgets/test_menu.py 75.82% <61.53%> (+0.33%) ⬆️
tests/conftest.py 87.50% <71.42%> (-10.18%) ⬇️
src/napari_imagej/__init__.py 82.60% <81.81%> (-17.40%) ⬇️
src/napari_imagej/resources/__init__.py 85.71% <85.71%> (ø)
src/napari_imagej/widgets/menu.py 85.95% <91.54%> (+3.99%) ⬆️
src/napari_imagej/java.py 95.23% <100.00%> (-0.07%) ⬇️
src/napari_imagej/widgets/napari_imagej.py 88.37% <100.00%> (+0.56%) ⬆️
tests/utils.py 96.33% <100.00%> (+0.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Nice! Just some minor comments. Feel free to act on or ignore them as you see fit.

src/napari_imagej/settings.py Outdated Show resolved Hide resolved
src/napari_imagej/settings.py Outdated Show resolved Hide resolved
src/napari_imagej/settings.toml Outdated Show resolved Hide resolved
src/napari_imagej/java.py Outdated Show resolved Hide resolved
src/napari_imagej/widgets/resources/__init__.py Outdated Show resolved Hide resolved
@gselzer gselzer marked this pull request as ready for review September 7, 2022 21:20
@ctrueden
Copy link
Member

ctrueden commented Sep 7, 2022

@gselzer This is looking really nice!

I ran into a couple of problems, like one does:

JVMNotRunning

I tried changing net.imagej:imagej to sc.fiji:fiji and restarting napari. Then when I run napari-imagej, I get the following stack trace:

---------------------------------------------------------------------------
JVMNotRunning                             Traceback (most recent call last)
File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/threading.py:1016, in Thread._bootstrap_inner(self=<Thread(Thread-8 (_init_producer), stopped 123145615675392)>)
   1013     _sys.setprofile(_profile_hook)
   1015 try:
-> 1016     self.run()
        self = <Thread(Thread-8 (_init_producer), stopped 123145615675392)>
   1017 except:
   1018     self._invoke_excepthook(self)

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/threading.py:953, in Thread.run(self=<Thread(Thread-8 (_init_producer), stopped 123145615675392)>)
    951 try:
    952     if self._target is not None:
--> 953         self._target(*self._args, **self._kwargs)
        self = <Thread(Thread-8 (_init_producer), stopped 123145615675392)>
    954 finally:
    955     # Avoid a refcycle if the thread is running a function with
    956     # an argument that has a member that points to the thread.
    957     del self._target, self._args, self._kwargs

File /Volumes/Code/napari/napari-imagej/src/napari_imagej/widgets/result_tree.py:156, in SearchResultTree._init_producer(self=<napari_imagej.widgets.result_tree.SearchResultTree object>)
    152 ensure_jvm_started()
    154 # Then, define our SearchListener
    155 @JImplements("org.scijava.search.SearchListener")
--> 156 class NapariImageJSearchListener:
    157     def __init__(self, event_handler: Signal):
    158         super().__init__()

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/site-packages/jpype/_jproxy.py:136, in JImplements.<locals>.JProxyCreator(cls=<class 'napari_imagej.widgets.result_tree.Search...it_producer.<locals>.NapariImageJSearchListener'>)
    135 def JProxyCreator(cls):
--> 136     return _createJProxy(cls, *interfaces, **kwargs)
        cls = <class 'napari_imagej.widgets.result_tree.SearchResultTree._init_producer.<locals>.NapariImageJSearchListener'>
        interfaces = ('org.scijava.search.SearchListener',)
        kwargs = {}

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/site-packages/jpype/_jproxy.py:80, in _createJProxy(cls=<class 'napari_imagej.widgets.result_tree.Search...it_producer.<locals>.NapariImageJSearchListener'>, *intf=('org.scijava.search.SearchListener',))
     76 def _createJProxy(cls, *intf):
     77     """ (internal) Create a proxy from a Python class with
     78     @JOverride notation on methods evaluated at declaration.
     79     """
---> 80     actualIntf = _prepareInterfaces(cls, intf)
        cls = <class 'napari_imagej.widgets.result_tree.SearchResultTree._init_producer.<locals>.NapariImageJSearchListener'>
        intf = ('org.scijava.search.SearchListener',)
     82     def new(tp, *args, **kwargs):
     83         self = _jpype._JProxy.__new__(tp, None, actualIntf)

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/site-packages/jpype/_jproxy.py:51, in _prepareInterfaces(cls=<class 'napari_imagej.widgets.result_tree.Search...it_producer.<locals>.NapariImageJSearchListener'>, intf=('org.scijava.search.SearchListener',))
     49 def _prepareInterfaces(cls, intf):
     50     # Convert the interfaces list
---> 51     actualIntf = _convertInterfaces(intf)
        intf = ('org.scijava.search.SearchListener',)
     52     overrides = _classOverrides(cls)
     53     _checkInterfaceOverrides(actualIntf, overrides)

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/site-packages/jpype/_jproxy.py:156, in _convertInterfaces(intf=('org.scijava.search.SearchListener',))
    154 for item in intflist:
    155     if isinstance(item, str):
--> 156         actualIntf.add(_jpype.JClass(item))
        item = 'org.scijava.search.SearchListener'
        actualIntf = set()
        _jpype.JClass = <class 'jpype._jclass.JClass'>
    157     else:
    158         actualIntf.add(item)

File ~/mambaforge/envs/napari-imagej-dev/lib/python3.10/site-packages/jpype/_jclass.py:99, in JClass.__new__(cls=<class 'jpype._jclass.JClass'>, jc='org.scijava.search.SearchListener', loader=None, initialize=True)
     96     return ret
     98 # Pass to class factory to create the type
---> 99 return _jpype._getClass(jc)
        jc = 'org.scijava.search.SearchListener'

JVMNotRunning: Java Virtual Machine is not running

Something weird with the multithreading, perhaps? Edit: it's just a symptom of the actual problem, which was emitted on the console:

Console error
16:32:26 DEBUG napari-imagej: Initializing ImageJ2
16:32:26 DEBUG napari-imagej: Completed JVM Configuration
16:32:27 ERROR Failed to bootstrap the artifact.
16:32:27 ERROR
16:32:27 ERROR Possible solutions:
16:32:27 ERROR * Double check the endpoint for correctness (https://search.maven.org/).
16:32:27 ERROR * Add needed repositories to ~/.jgorc [repositories] block (see README).
16:32:27 ERROR * Try with an explicit version number (release metadata might be wrong).
16:32:27 ERROR
16:32:27 ERROR Full Maven error output:
16:32:27 ERROR 	[ERROR] [ERROR] Some problems were encountered while processing the POMs:
16:32:27 ERROR 	[ERROR] Non-resolvable import POM: Failed to resolve version for sc.fiji:fiji:pom:RELEASE @ line 8, column 29
16:32:27 ERROR 	[ERROR] The build could not read 1 project -> [Help 1]
16:32:27 ERROR 	[ERROR]
16:32:27 ERROR 	[ERROR]   The project sc.fiji-BOOTSTRAPPER:fiji-BOOTSTRAPPER:0 (/Users/curtis/.jgo/sc.fiji/fiji/RELEASE/b616c577c2873bd4b5338a8fad1280c9b5448e17fda031f6b7a4fc8612418b0b/pom.xml) has 1 error
16:32:27 ERROR 	[ERROR]     Non-resolvable import POM: Failed to resolve version for sc.fiji:fiji:pom:RELEASE @ line 8, column 29 -> [Help 2]
16:32:27 ERROR 	[ERROR]
16:32:27 ERROR 	[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
16:32:27 ERROR 	[ERROR] Re-run Maven using the -X switch to enable full debug logging.
16:32:27 ERROR 	[ERROR]
16:32:27 ERROR 	[ERROR] For more information about the errors and possible solutions, please read the following articles:
16:32:27 ERROR 	[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
16:32:27 ERROR 	[ERROR] [Help 2] http://cwiki.apache.org/confluence/display/MAVEN/UnresolvableModelException
16:32:27 ERROR

Suboptimal dialog message when clicking ImageJ GUI button on macOS

The message I receive is: "The ImageJ2 user interface cannot be opened when running PyImageJ headlessly. Visit this site for more information." Which is correct, but what is not explained is that because I'm on macOS, napari-imagej has no choice but to run in headless mode, and interactive mode is not available. I think this could be better communicated to users.

Relatedly: I'm not sure if on non-Mac platforms, the settings dialog has a toggle between headless and interactive, but I don't see it on macOS.

@ctrueden
Copy link
Member

ctrueden commented Sep 7, 2022

I looked into why the sc.fiji:fiji endpoint failed to resolve with Maven.

If I change RELEASE to 2.6.0 in the generated pom.xml, it works.

I was worried that Maven finally killed support for the special RELEASE keyword once and for all... but after rm -rf ~/.m2/repository/sc/fiji/fiji, doing mvn dependency:get -Dartifact=sc.fiji:fiji:RELEASE fetched the previously missing maven-metadata.xml and the resolution worked. So 🤷, not actually an issue.

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

LGTM!

@gselzer
Copy link
Collaborator Author

gselzer commented Sep 8, 2022

A couple things I'd like to add before merge:

  • For some reason, the ordering of the settings is not the same every time. It seems to change based on the value of the settings. If I start out with choose_active_layer (ordered second in the list) checked, and uncheck it, the next time I click the settings button it will appear first! And then, if I check it again, it will then appear second if I click on the settings button a third time!
  • I'd like to add a disclaimer that settings changes will only take effect on restart. But I'm not sure that napari's request_values dialog has the option for this, so maybe I'll add this disclaimer in a popup that spawns only if the user makes any changes to the settings.
  • I'd like to test that pressing the settings button and making a change will actually affect the file.

Maybe later we could use the new settings_change signal to incorporate
some of the changes without a restart, but a restart is unavoidable for
other settings (the imagej directory/endpoint being the poster child for
this)
Due to the way confuse maintains settings changes, we can get different
settings orderings in the popup as a function of the previous edits
made.

By editing over the settings present in the DEFAULT source
(src/napari-imagej/config-default.yaml), we get two nice effects:
1. Settings will always appear in the order listed in the default
configuration file
2. Defined settings that are NOT in the default configuration file are
not listed in this popup. This could happen if the user hacked their own
configuration, for example.

Unfortunately, this does not fix the way that settings changes are
dumped into the user's settings. Thus the settings can still appear out
of order in the user's file. More work needed on that one...
This change was motivated by @imagejan's comments, but I also took the
liberty of documenting the function
Copy link
Collaborator Author

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

I only got through part of it, but gotta run now. Will do the rest later!

.gitignore Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
src/napari_imagej/__init__.py Outdated Show resolved Hide resolved
src/napari_imagej/java.py Outdated Show resolved Hide resolved
src/napari_imagej/java.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

Finished the review!

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/utils.py Show resolved Hide resolved
tests/widgets/test_menu.py Outdated Show resolved Hide resolved
tests/widgets/test_menu.py Show resolved Hide resolved
tests/widgets/test_menu.py Outdated Show resolved Hide resolved
@gselzer gselzer force-pushed the 98-settings-refactor branch 3 times, most recently from fb645e8 to 5dca3dc Compare September 9, 2022 20:29
@gselzer gselzer force-pushed the 98-settings-refactor branch 10 times, most recently from 52f4b22 to 4dfc6ac Compare September 13, 2022 15:08
@gselzer gselzer merged commit d47796a into main Sep 13, 2022
@gselzer gselzer deleted the 98-settings-refactor branch September 13, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'settings.yml' not found : the plugin cannot be started
4 participants