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

70 Create a GUI entrypoint #75

Merged
merged 1 commit into from
Jul 22, 2023
Merged

Conversation

paulhfu
Copy link
Collaborator

@paulhfu paulhfu commented Jul 16, 2023

Fixes 70

So far only 2d annotator is implemented.

@paulhfu paulhfu self-assigned this Jul 16, 2023
@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 16, 2023

@constantinpape could you check this first version for the 2d annotator?
Once approved I can add the other workflows.

@constantinpape
Copy link
Contributor

constantinpape commented Jul 16, 2023

Hi @paulhfu,
the GUI itself looks good, but I have a few other comments and change requests:

Annotator does not start due to import error

For me the 2d annotator does not start because of a vigra import error:

Traceback (most recent call last):
  File "/home/pape/Work/my_projects/micro-sam/micro_sam/gui_entry.py", line 277, in <module>
    from micro_sam.sam_annotator import annotator_2d
  File "/home/pape/Work/my_projects/micro-sam/micro_sam/sam_annotator/__init__.py", line 1, in <module>
    from .annotator_2d import annotator_2d
  File "/home/pape/Work/my_projects/micro-sam/micro_sam/sam_annotator/annotator_2d.py", line 9, in <module>
    from .. import util
  File "/home/pape/Work/my_projects/micro-sam/micro_sam/util.py", line 9, in <module>
    import vigra
  File "/home/pape/software/conda/miniconda3/envs/sam/lib/python3.10/site-packages/vigra/__init__.py", line 118, in <module>
    import vigra.analysis as analysis
ImportError: /lib/x86_64-linux-gnu/libtinfo.so.6: version `NCURSES6_TINFO_6.2.20211010' not found (required by /home/pape/software/conda/miniconda3/envs/sam/lib/python3.10/site-packages/vigra/../../../././libncurses.so.6)
free(): invalid pointer
Aborted (core dumped)

I think this can be avoided by refactoring the GUI as follows:

  • Move the file gui_entry.py to sam_annotator/annotator.py (the filename itself ofc doesn't matter, but for consistency I'd like to call it annotator).
  • Use from .sam_annotator_2d import sam_anntoator_2d etc. instead of the absolute import
  • Instead of using __main__ as an entrypoint write a main function that serves as entrypoint and expose it via CLI in the setup.py. See the examples for the other entrypoints here: https://github.com/computational-cell-analytics/micro-sam/blob/master/setup.py#L14-L21

Warnings about stylefiles

Also, I get the following warnings when starting the GUI:

WARNING: Cannot open file '/home/pape/Work/my_projects/micro-sam/micro_sam/theme_dark:/drop_down_50.svg', because: No such file or directory
WARNING: Cannot open file '/home/pape/Work/my_projects/micro-sam/micro_sam/theme_dark:/drop_down_50.svg', because: No such file or directory

Linter

Finally: my linter is not so happy when looking at your file ;). It would be great if you could get a PEP8 compatible linter set-up and run it on your files. For referene, I am using flake8.
(This is not so high priority for the next changes, it's fine to do is when we finalize this PR.)

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 17, 2023

Hi @constantinpape, next time I will clean the code before comitting.
I will also check the source of those warnings. I cannot see them on my system. Is the napari dark theme working for you?
There are some issues with moving the file. I had a hard time separating the magicgui widgets from my QT event loop.
My guess is, that the magicgui widgets are constructed when then decorators are executed, which seems to be at import time.
If the event loop is then startet, the QWidgets will be registered and deleted when the event loop is destructed.
This leads to errors when we try to dock the widgets in napari.
If I put the file into the sam_annotator module, importing it triggers the magicgui decorators which have dependecies to init.py.

I will investigate a bit more, maybe there is a simple solution.

@constantinpape
Copy link
Contributor

next time I will clean the code before comitting.

Thanks!

There are some issues with moving the file. I had a hard time separating the magicgui widgets from my QT event loop.
My guess is, that the magicgui widgets are constructed when then decorators are executed, which seems to be at import time.
If the event loop is then startet, the QWidgets will be registered and deleted when the event loop is destructed.
This leads to errors when we try to dock the widgets in napari.

Ok. A possible solution would maybe be to do these GUIs in magicgui as well. (It can be used standalone and does not depend on napari).

I will also check the source of those warnings. I cannot see them on my system. Is the napari dark theme working for you?

I think that the dark theme is working. Anyways, the warnings are not so important for now; it's more important to get the GUI itself working first ;) .

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 17, 2023

Ok. A possible solution would maybe be to do these GUIs in magicgui as well. (It can be used standalone and does not depend on napari).

I tried also a magicgui version and there the examples from the website where just showing black windows without event loop.
I had to start the event loop manually for them to work. Which brings us back to the original issue.
Are the examples working for you? Maybe it is my system, I am getting this
qt.qpa.plugin: Could not find the Qt platform plugin "wayland" in ""
warnings. Maybe that is the issue.

@constantinpape
Copy link
Contributor

Are the examples working for you?

Can you point me to which example you mean exactly?

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 17, 2023

I copied some code snippeds from here:
https://pyapp-kit.github.io/magicgui/
They work for me only if I wrap them in a QApplication.
Same for this demo:
https://pyapp-kit.github.io/magicgui/examples/basic/

If I make a new env with only magicgui and its upstream dependencies in it not even the black windows is showing and I see this warning:
QSocketNotifier: Can only be used with threads started with QThread

@constantinpape
Copy link
Contributor

Thanks for clarifying, I will check it tomorrow morning.

@constantinpape
Copy link
Contributor

It's not working for me either, see pyapp-kit/magicgui#566 .

@tlambert03
Copy link

Regarding the widget creation when using the decorator: yes... the decorator creates a widget. Use magic_factory if you don't immediately want a widget... see more here:

https://pyapp-kit.github.io/magicgui/decorators/#magic_factory

As for the examples, if you're running in a script, add .show(run=True).

Will update docs

@constantinpape
Copy link
Contributor

Thanks for the clarification here as well @tlambert03 !

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 18, 2023

Thanks a lot @tlambert03. It is working now.
Using magicgui it would look kind of like this:

image

Maybe I could add some more refinements. I could not center-align the header.
Would this be ok?

@constantinpape
Copy link
Contributor

Thanks @paulhfu! This looks ok for now. Let's go ahead with it and see if the whole mechanism works now.
We can look into improving the GUI look a bit later.

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 20, 2023

Hi @constantinpape,
I made the whole gui with magicgui now. This is not ready to be reviewed or functionally tested.
I will add refinements tomorrow. But if you want to have a look at the structure, please go ahead.

What version of flake8 are you using and do you have a config file for it? I am getting a lot of errors in all files. Mostly "line too long" but also others. I am using v2023.6.0 with the default configs.

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

I gave this a very quick view, and the structure looks good. I will have a closer look once you say it's ready for review.

sub_widget.show()


def main():
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this annotator to be consistent with the other files.

@constantinpape
Copy link
Contributor

What version of flake8 are you using and do you have a config file for it? I am getting a lot of errors in all files. Mostly "line too long" but also others. I am using v2023.6.0 with the default configs.

I am using the default settings, except for --max-line-length=120 (otherwise it's set to 80, which is not quite enough for readable code.)
My env has

flake8                    4.0.1              pyhd8ed1ab_1    conda-forge

Let me know if there's any more significant warnings for you after setting the line length to 120, then I can have another look at this!

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 22, 2023

@constantinpape thank you for the config. Looks much better now.
I dont know what is evaluated as significant, there is a missing import and an indentation issue.
Here are all the warnings and errors I am seeing after your setting (this are the results from this branch):

[{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E302",
	"severity": 8,
	"message": "expected 2 blank lines, found 1",
	"source": "Flake8",
	"startLineNumber": 319,
	"startColumn": 1,
	"endLineNumber": 319,
	"endColumn": 1
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (125 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 320,
	"startColumn": 121,
	"endLineNumber": 320,
	"endColumn": 121
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E203",
	"severity": 8,
	"message": "whitespace before ','",
	"source": "Flake8",
	"startLineNumber": 336,
	"startColumn": 43,
	"endLineNumber": 336,
	"endColumn": 43
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E203",
	"severity": 8,
	"message": "whitespace before ','",
	"source": "Flake8",
	"startLineNumber": 337,
	"startColumn": 54,
	"endLineNumber": 337,
	"endColumn": 54
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E203",
	"severity": 8,
	"message": "whitespace before ','",
	"source": "Flake8",
	"startLineNumber": 338,
	"startColumn": 58,
	"endLineNumber": 338,
	"endColumn": 58
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E231",
	"severity": 8,
	"message": "missing whitespace after ','",
	"source": "Flake8",
	"startLineNumber": 338,
	"startColumn": 59,
	"endLineNumber": 338,
	"endColumn": 59
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E203",
	"severity": 8,
	"message": "whitespace before ','",
	"source": "Flake8",
	"startLineNumber": 339,
	"startColumn": 52,
	"endLineNumber": 339,
	"endColumn": 52
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E231",
	"severity": 8,
	"message": "missing whitespace after ','",
	"source": "Flake8",
	"startLineNumber": 339,
	"startColumn": 53,
	"endLineNumber": 339,
	"endColumn": 53
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (134 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 350,
	"startColumn": 121,
	"endLineNumber": 350,
	"endColumn": 121
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E275",
	"severity": 8,
	"message": "missing whitespace after keyword",
	"source": "Flake8",
	"startLineNumber": 360,
	"startColumn": 15,
	"endLineNumber": 360,
	"endColumn": 15
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (145 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 362,
	"startColumn": 121,
	"endLineNumber": 362,
	"endColumn": 121
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E111",
	"severity": 8,
	"message": "indentation is not a multiple of 4",
	"source": "Flake8",
	"startLineNumber": 398,
	"startColumn": 20,
	"endLineNumber": 398,
	"endColumn": 20
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "W293",
	"severity": 4,
	"message": "blank line contains whitespace",
	"source": "Flake8",
	"startLineNumber": 325,
	"startColumn": 1,
	"endLineNumber": 325,
	"endColumn": 1
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "W291",
	"severity": 4,
	"message": "trailing whitespace",
	"source": "Flake8",
	"startLineNumber": 348,
	"startColumn": 25,
	"endLineNumber": 348,
	"endColumn": 25
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "W293",
	"severity": 4,
	"message": "blank line contains whitespace",
	"source": "Flake8",
	"startLineNumber": 387,
	"startColumn": 1,
	"endLineNumber": 387,
	"endColumn": 1
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "W293",
	"severity": 4,
	"message": "blank line contains whitespace",
	"source": "Flake8",
	"startLineNumber": 390,
	"startColumn": 1,
	"endLineNumber": 390,
	"endColumn": 1
},{
	"resource": "micro-sam/micro_sam/util.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "W291",
	"severity": 4,
	"message": "trailing whitespace",
	"source": "Flake8",
	"startLineNumber": 398,
	"startColumn": 62,
	"endLineNumber": 398,
	"endColumn": 62
}]

[{
	"resource": "micro-sam/micro_sam/sam_annotator/image_series_annotator.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "F821",
	"severity": 8,
	"message": "undefined name 'warnings'",
	"source": "Flake8",
	"startLineNumber": 131,
	"startColumn": 9,
	"endLineNumber": 131,
	"endColumn": 9
},{
	"resource": "micro-sam/micro_sam/sam_annotator/image_series_annotator.py",
	"owner": "_generated_diagnostic_collection_name_#2",
	"code": {
		"value": "reportUndefinedVariable",
		"target": {
			"$mid": 1,
			"path": "/microsoft/pyright/blob/main/docs/configuration.md",
			"scheme": "https",
			"authority": "github.com",
			"fragment": "reportUndefinedVariable"
		}
	},
	"severity": 4,
	"message": "\"warnings\" is not defined",
	"source": "Pylance",
	"startLineNumber": 131,
	"startColumn": 9,
	"endLineNumber": 131,
	"endColumn": 17
}]

[{
	"resource": "micro-sam/micro_sam/prompt_generators.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (124 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 33,
	"startColumn": 121,
	"endLineNumber": 33,
	"endColumn": 121
},{
	"resource": "micro-sam/micro_sam/prompt_generators.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (142 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 54,
	"startColumn": 121,
	"endLineNumber": 54,
	"endColumn": 121
},{
	"resource": "micro-sam/micro_sam/prompt_generators.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "E501",
	"severity": 8,
	"message": "line too long (142 > 120 characters)",
	"source": "Flake8",
	"startLineNumber": 84,
	"startColumn": 121,
	"endLineNumber": 84,
	"endColumn": 121
}]

[{
	"resource": "micro-sam/examples/sam_annotator_3d.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "F841",
	"severity": 8,
	"message": "local variable 'fnames' is assigned to but never used",
	"source": "Flake8",
	"startLineNumber": 26,
	"startColumn": 5,
	"endLineNumber": 26,
	"endColumn": 5
}]

[{
	"resource": "micro-sam/examples/sam_annotator_tracking.py",
	"owner": "_generated_diagnostic_collection_name_#5",
	"code": "F841",
	"severity": 8,
	"message": "local variable 'fnames' is assigned to but never used",
	"source": "Flake8",
	"startLineNumber": 32,
	"startColumn": 5,
	"endLineNumber": 32,
	"endColumn": 5
}]

@constantinpape
Copy link
Contributor

@paulhfu all of these should be fixed if you merge the current master into this branch.

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 22, 2023

@constantinpape yes, thank you, I rebased.
Except for the missing import of warnings in image_series_annotator.py it looks good now.
Should I add this import here? Not sure since it it not part of the fixed issue.

@constantinpape
Copy link
Contributor

Except for the missing import of warnings in image_series_annotator.py it looks good now.
Should I add this import here? Not sure since it it not part of the fixed issue.

You're right, that's still missing.
That must have somehow slipped through in my recent changes!

Please go ahead and add the import here.

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 22, 2023

@constantinpape, this can now be reviewed. I could not finish all my tests, I will continue later.
I rebased again (in case you have checked out the branch).

Open questions from my side are:

  • Stylesheet. Use with warnings or not use it? (I could not eliminate the warnings)
  • Grayscale is disabled for 3D annotation. Why?
  • Is it ok to open explicit image paths with imageio.imread and whole directories with elf.io.open_file?
  • Are the dimensions of halo and tile in the gui correct in the 4 annotators?

Open Issues:

  • 2d annotation with one image+segentaionm result ->I can see a warning coming from two simultaneously open QApplications and after that napari closes.
/home/drford/Documents/micro-sam/micro_sam/sam_annotator/annotator_2d.py:243: UserWarning: A QApplication is already running with 1 event loop. To enter *another* event loop, use `run(max_loop_level=2)`
  napari.run()

@constantinpape
Copy link
Contributor

Thanks @paulhfu ! Looks very good overall.

Before I go through your questions: one more thing I noticed that is missing in the 2d annotator, 3d annotator and tracking_annotator is the (optional) input_key, so that we can also open data from hdf5, zarr or n5.

  • Stylesheet. Use with warnings or not use it? (I could not eliminate the warnings)

I don't see that particular warning anymore. Let's keep the stylesheet for now, we can look closer into this later if it comes up again.

Grayscale is disabled for 3D annotation. Why?

I am not quite sure what you mean. Can you give an example?

Is it ok to open explicit image paths with imageio.imread and whole directories with elf.io.open_file?

Yes, that's ok. You could also use https://github.com/computational-cell-analytics/micro-sam/blob/master/micro_sam/util.py#L480 . It does the same thing you describe, but you wouldn't need to duplicate that code, and maybe I have time to do something smarter / extend support for more file formats there at some point.

  • Are the dimensions of halo and tile in the gui correct in the 4 annotators?

No, tiling is always done just in 2d. So it should be just Tile x, Tile y, Halo x, Halo y in all the 4 annotators.

  • 2d annotation with one image+segentaionm result ->I can see a warning coming from two simultaneously open QApplications and after that napari closes.

I also saw that warning pop up once. But for now I would suggest that we do not tackle this in the current PR but rather leave it for follow up work. I want to publish a new release next week, and once you have addressed the two main remaining issues (input_key and tiling / halo) I would merge it so that it can be included in the release; since this is already the most user-friendly way of starting micro_sam :) .

@paulhfu
Copy link
Collaborator Author

paulhfu commented Jul 22, 2023

Hi @constantinpape,

I am not quite sure what you mean. Can you give an example?

I got errors which stated something along the lines "Grayscale is not supported for 3D annotation. User RGB"
This is not the case anymore, it is working now. Either it was the Tiling/Halo issue or it was fixed in master.

I applied your suggestions.

@constantinpape
Copy link
Contributor

I got errors which stated something along the lines "Grayscale is not supported for 3D annotation. User RGB"
This is not the case anymore, it is working now. Either it was the Tiling/Halo issue or it was fixed in master.

Yeah, I think I fixed that on master.

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

I will go ahead and merge this now. The base functionality is working, and we can improve it in potential follow-ups. Thanks!

@constantinpape constantinpape merged commit b59654c into master Jul 22, 2023
2 checks passed
@@ -13,6 +13,7 @@
license="MIT",
entry_points={
"console_scripts": [
"micro_sam.annotator = micro_sam.sam_annotator.annotator:main",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@constantinpape, will this entry not be part in the conda-forge review?

Copy link
Contributor

@constantinpape constantinpape Jul 24, 2023

Choose a reason for hiding this comment

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

It will be part of conda-forge, but I will need to publish a new release of micro_sam first. Will try to do that tomorrow morning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Got it.

@constantinpape constantinpape deleted the 70_create_GUI_entry_point branch May 5, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants