Add preferences dialog #2899

k-dominik · 2024-08-21T15:33:08Z

This PR makes the settings, so far governed only by ilastik.ini, accessible to users via the UI:

In the process I introduced a config class based on pydantic. The settings, edited via the dialog, will be written to ilastik.ini - this is fully compatible with the settings file so far. Note: Only settings different to default values are written to the file.

Currently a full config would look like this:

Example ilastik.ini

[ilastik]
plugin_directories = ~/my_plugins
output_filename_format = {dataset_dir}/{nickname}_output_{result_type}
output_format = tiff
logging_config = /path/to/some/logging_config.json
debug = True
hbp = True

[lazyflow]
threads = 50
total_ram_mb = 10000

Todos:

reading/writing of config
user-friendly tooltips/validation
investigate pydantic (pydantic-settings, maybe) 🤡
in the end pydantic-settings wasn't worth it - we don't want most of it's features (no need for .env files, we don't want to expose every setting via env variables, and also not via argparse (let alone that all of these would need to be customized to integrate with our current "interface")

Checklist

Format code and imports.
Add docstrings and comments.
Add tests.
Update user documentation. -> Added documentation for preferences dialog ilastik.github.io#278
Reference relevant issues and other pull requests.
Rebase commits into a logical sequence.

Fixes #2863

codecov · 2024-08-21T16:03:20Z

Codecov Report

Attention: Patch coverage is 69.54733% with 74 lines in your changes missing coverage. Please review.

Project coverage is 56.56%. Comparing base (cca71b7) to head (39eaca2).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...tik/applets/tracking/annotations/annotationsGui.py	0.00%	10 Missing ⚠️
ilastik/shell/gui/preferencesDialog.py	93.89%	6 Missing and 2 partials ⚠️
...astik/applets/tracking/manual/manualTrackingGui.py	0.00%	7 Missing ⚠️
ilastik/app.py	25.00%	2 Missing and 4 partials ⚠️
ilastik/config.py	84.61%	5 Missing and 1 partial ⚠️
ilastik/shell/gui/ilastikShell.py	40.00%	2 Missing and 4 partials ⚠️
ilastik/widgets/stackFileSelectionWidget.py	0.00%	3 Missing ⚠️
ilastik/workflows/__init__.py	0.00%	0 Missing and 3 partials ⚠️
ilastik/applets/edgeTraining/edgeTrainingGui.py	0.00%	0 Missing and 2 partials ⚠️
...ik/applets/objectExtraction/objectExtractionGui.py	0.00%	1 Missing and 1 partial ⚠️
... and 16 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2899      +/-   ##
==========================================
+ Coverage   56.45%   56.56%   +0.10%     
==========================================
  Files         535      536       +1     
  Lines       62264    62420     +156     
  Branches     7711     7729      +18     
==========================================
+ Hits        35154    35309     +155     
- Misses      25345    25348       +3     
+ Partials     1765     1763       -2

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

btbest

Edit: This was meant to be "Request changes" because preferencesDialog.py has a bug in an else-raise-clause due to variable naming, default_config should be deleted (I think) and the flat_dict should be explained or removed.

Really nice, the widget generator and the prefs based on pydantic will make it pretty easy to extend the configurable options in the future.

I see a potential for confusion because we have a preferences.json, which the user can locate with Open Config Folder - but these are the volumina preferences. Now we have a Preferences dialog that controls a different set of preferences that we internally call config.

From the dev perspective, it might be worth just adding a short note to the respective docstrings to explain that ilastik-config goes with PreferencesDialog and that these are separate from preferences, which is from volumina. Though the code is easy enough to trace anyway.

From the user perspective, I'm not really sure what can be done. Maybe creating the ilastik.ini file with the default values could help, so that at least if a user uses Open Config Folder, they can see that there are in fact two separate sets of settings?

ilastik/config.py

ilastik/shell/gui/preferencesDialog.py

ilastik/config.py

tests/test_ilastik/test_shell/test_PreferencesDialog.py

ilastik/config.py

* removed flattening of config dict - just works without * removed `default_config` (not used) -> moved information docstring as example * removed qtbot fixture from test that don't need it * renamed `Increment.increment` member to `inc` to avoid confusion Co-authored-by: Benedikt Best <[email protected]>

Co-authored-by: Benedikt Best <[email protected]>

Some options are not validated very well. For now we hide those to prevent changing them without much consideration.

btbest

:/

I realised there's an issue with coupling the dialog's model directly to the in-use runtime config: Regardless of whether you Ok or Cancel the dialog (which governs whether you write to disk), any non-default values you entered in the inputs are kept in the active runtime config. Also, if a non-default value was read from file, then resetting it to default in the dialog and clicking Ok doesn't actually transfer the value to the model.

The dialog does tell the user to please restart ilastik, but that should only be a precaution and not a requirement in order for Ok and Cancel to modify (or not modify) the ongoing runtime configs...

Request changes because there's also a remaining usage of the old config dict syntax in dataExportGui: settingsDlg = DataExportOptionsDlg(self, opExportModelOp, cfg["ilastik"]["output_filename_format"])

btbest · 2024-12-03T16:12:39Z

ilastik/shell/gui/preferencesDialog.py

+        return widget, advanced
+
+    def initUI(self):
+        self._main_layout = QVBoxLayout()


Now that I look at it again, I think all the layouts and widgets instantiated here can be local variables instead of self. properties, as they're not referenced in any other method or outside the class 😅

btbest · 2024-12-03T16:14:55Z

ilastik/shell/gui/preferencesDialog.py

+        self.buttonBox = QDialogButtonBox(QDialogButtonBox.Ok | QDialogButtonBox.Cancel)
+        self.resetButton = QPushButton("Reset to Defaults")


As far as I can see, buttonBox and resetButton aren't referenced outside the class, so prefix _? (Or as suggested in the other comment, make them all local variables only?)

btbest · 2024-12-03T16:15:16Z

ilastik/shell/gui/preferencesDialog.py

+        # adv_chbox_layout = QHBoxLayout()
+        self._adv_checkbox = QCheckBox("Show advanced options")
+        self._adv_checkbox.setTristate(False)
+        self._adv_checkbox.stateChanged.connect(lambda x: self._update_advanced_view(x != 2))


Suggested change

# adv_chbox_layout = QHBoxLayout()

self._adv_checkbox = QCheckBox("Show advanced options")

self._adv_checkbox.setTristate(False)

self._adv_checkbox.stateChanged.connect(lambda x: self._update_advanced_view(x != 2))

self._adv_checkbox = QCheckBox("Show advanced options")

self._adv_checkbox.setTristate(False)

self._adv_checkbox.stateChanged.connect(lambda state: self._update_advanced_view(state != Qt.Checked))

btbest · 2024-12-03T16:40:52Z

ilastik/shell/gui/preferencesDialog.py

+        pref_dict = self.config.model_dump(exclude_defaults=True)
+        cf = configparser.ConfigParser()
+        cf.read_dict(pref_dict)


Suggested change

pref_dict = self.config.model_dump(exclude_defaults=True)

cf = configparser.ConfigParser()

cf.read_dict(pref_dict)

unvalidated = self.config.model_dump(exclude_defaults=True)

self.config = self.config.model_validate(unvalidated)

validated = self.config.model_dump(exclude_defaults=True)

cf = configparser.ConfigParser()

cf.read_dict(validated)

So that the stripped results are saved to file

k-dominik force-pushed the settings-dlg branch 9 times, most recently from 563948b to 548c6b9 Compare August 28, 2024 06:25

k-dominik marked this pull request as ready for review August 28, 2024 07:03

k-dominik added 4 commits August 28, 2024 20:38

make default config path to always have a default

11aa77c

Pydantic-based ilastik settings

18dee44

Added basic preferences dialog

9ae2187

Added tests for preference dialog

d92f96a

k-dominik force-pushed the settings-dlg branch from 548c6b9 to d92f96a Compare August 28, 2024 18:39

btbest reviewed Sep 5, 2024

View reviewed changes

k-dominik and others added 4 commits December 2, 2024 17:18

Strip whitespace from string options

bda48e7

Co-authored-by: Benedikt Best <[email protected]>

Add advanced options - hidden by default

5bc9055

Some options are not validated very well. For now we hide those to prevent changing them without much consideration.

Merge branch 'main' into settings-dlg

39eaca2

k-dominik force-pushed the settings-dlg branch from 3141217 to 39eaca2 Compare December 3, 2024 08:59

k-dominik requested a review from btbest December 3, 2024 15:21

btbest requested changes Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preferences dialog #2899

Add preferences dialog #2899

k-dominik commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

btbest left a comment •

edited

Loading

btbest left a comment

btbest Dec 3, 2024 •

edited

Loading

btbest Dec 3, 2024

btbest Dec 3, 2024

btbest Dec 3, 2024

		self.buttonBox = QDialogButtonBox(QDialogButtonBox.Ok \| QDialogButtonBox.Cancel)
		self.resetButton = QPushButton("Reset to Defaults")

Add preferences dialog #2899

Are you sure you want to change the base?

Add preferences dialog #2899

Conversation

k-dominik commented Aug 21, 2024 • edited Loading

Checklist

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

btbest left a comment • edited Loading

Choose a reason for hiding this comment

btbest left a comment

Choose a reason for hiding this comment

btbest Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

btbest Dec 3, 2024

Choose a reason for hiding this comment

btbest Dec 3, 2024

Choose a reason for hiding this comment

btbest Dec 3, 2024

Choose a reason for hiding this comment

k-dominik commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

btbest left a comment •

edited

Loading

btbest Dec 3, 2024 •

edited

Loading