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

Add ConnectToSupervisely #2024

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Add ConnectToSupervisely #2024

merged 2 commits into from
Apr 18, 2024

Conversation

GoldenAnpu
Copy link
Contributor

New extension

  • Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • Repository name is Slicer+ExtensionName
  • Repository is associated with 3d-slicer-extension GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter 3d-slicer-extension in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topics
  • Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • Any known related patents must be mentioned in the extension description.
  • LICENSE.txt is present in the repository root. MIT (https://choosealicense.com/licenses/mit/) or Apache (https://choosealicense.com/licenses/apache-2.0/) license is recommended. If source code license is more restrictive for users than MIT, BSD, Apache, or 3D Slicer license then the name of the used license must be mentioned in the extension description.
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, release, ...) instead of a specific git hash to avoid re-submitting pull request whenever the extension is updated
  • Extension icon URL is correct (do not use the icon's webpage but the raw data download URL that you get from the download button - it should look something like this: https://raw.githubusercontent.com/user/repo/main/SomeIcon.png)
  • Screenshot URLs (screenshoturls) are correct, contains at least one
  • Homepage URL points to valid webpage containing the following:
    • Extension name
    • Short description: 1-2 sentences, which summarizes what the extension is usable for
    • At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • Description of contained modules: at one sentence for each module
    • Tutorial: step-by-step description of at least the most typical use case, include a few screenshots, provide download links to sample input data set
    • Publication: link to publication and/or to PubMed reference (if available)
    • License: We suggest you use a permissive license that includes patent and contribution clauses. This will help protect developers and ensure the code remains freely available. We suggest you use the Slicer License or the Apache 2.0. Always mention in your README file the license you have chosen. If you choose a different license, explain why to the extension maintainers. Depending on the license we may not be able to host your work. Read here to learn more about licenses.
    • Content of submitted s4ext file is consistent with the top-level CMakeLists.txt file in the repository (description, URLs, dependencies, etc. are the same)
  • Hide unused features in the repository to reduce noise/irrelevant information:
    • Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used)
    • Click the settings icon next to About in the top-right corner of the repository main page and uncheck Releases and Packages (if they are currently not used)

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2024

Thank you for your contribution! It is very impressive how the extension and the Supervisely backend work together.

  • I would recommended to consider improving the extension name - for me the order of words in the current AdapterSuperviselyCV does not feel natural. Something like SuperviselyAdapter or SuperviselyBridge would sound good.
  • It could make sense to include Supervisely in module names to make them easier to find in the module finder (Ctrl-F). Something like Supervisely Labeling and Supervisely Review.
  • Installation failed for me on Windows. I had the current pandas version (2.2.1) installed by some other extensions. When I installed the extension and switched to Labeling Jobs Annotation module, it installed some things but then it just stopped. It did not restart Slicer. I saw this in the log:
"pandas>=1.1.3, <=2.1.4",

  Attempting uninstall: pandas
    Found existing installation: pandas 2.2.1
    Uninstalling pandas-2.2.1:
      Successfully uninstalled pandas-2.2.1
ERROR: Could not install packages due to an OSError: [WinError 5] Access is denied: 'C:\\Users\\andra\\AppData\\Local\\slicer.org\\Slicer 5.7.0-2024-04-03\\lib\\Python\\Lib\\site-packages\\~andas.libs\\msvcp140-ef6047a69b174ada5cb2eff1d2bc9a62.dll'

The installation process should be made more robust and it should not stop when some package installation failed but restart Slicer and retry the installation.

It would be also nice to make Supervisely compatible with current pandas version.

  • In the Authentication tab, enter the required data -> I had trouble figuring out what to do here. Give simple instructions for registering, setting up a simple project, tell what server URL to use. By using the browser developer tools I figured out that app.supervisely.com is the server URL, but it should have been documented, or even better it should have been recommended by default.
  • I added task list, etc. but the GUI in Slicer did not refresh. I disconnected and connected, which helped, but it was annoying that I had to enter the server, username, password again from scratch. It would be nice to have a refresh button or make things automatically refresh.
  • I could not figure out the logic of how segmentations and segments are mapped. I tried to add tags, classes, objects in the web interface and segmentations and segments in Slicer and some segments were uploaded, but I still don't know what happened and why.
  • Tags did not show up for me:

image

  • I could not download segmentation from the server into Slicer. Is this expected?
  • Team and Job combobox is set to be editable, This makes it more difficult to make a selection.

@pieper
Copy link
Member

pieper commented Apr 12, 2024

Looks very cool 👍

  • AdapterSuperviselyCV does not feel natural. Something like SuperviselyAdapter or SuperviselyBridge would sound good.

I think SuperviselyClient would make sense.

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2024

Or SuperviselyConnector

@GoldenAnpu
Copy link
Contributor Author

Thank you very much for your detailed feedback 🤗
I agree with all of the above points. We will update and make extension more understandable for users unfamiliar with our platform.

In case of name change, we would like to use ConnectToSupervisely . Is it possible to use such naming?

@jamesobutler
Copy link
Contributor

ConnectToSupervisely

@GoldenAnpu Does this mean the module can only connect to Supervisely, but cannot disconnect from Supervisely? If both are supported then SuperviselyConnector would seem to be the better term that implies it could do both.

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2024

Also, it is beneficial to start the name with Supervisely because that what you want to emphasize; users may scroll to S when they look for a Supervisely extension; and you may release more extensions in the future that would all nicely show up together if their names all start with Supervisely (see for example FlywheelConnect and FlywheelCaseIterator extensions).

But ConnectToSupervisely would be fine for the Extensions Catalog if you prefer that.

@lassoan lassoan added the Status: Awaiting Response ⏳ Waiting for a response/more information label Apr 17, 2024
@GoldenAnpu GoldenAnpu changed the title Add AdapterSuperviselyCV Add ConnectToSupervisely Apr 18, 2024
@GoldenAnpu
Copy link
Contributor Author

I would recommended to consider improving the extension name - for me the order of words in the current AdapterSuperviselyCV does not feel natural. Something like SuperviselyAdapter or SuperviselyBridge would sound good.

The name ConnectToSupervisely was chosen. We like this name and it fully conveys the meaning of the extension. Users who are looking for our extension will always be able to find it through search field. In addition, it implies that it is placed in a separate Supervisely section where we go to see our extensions.
@jamesobutler we would like extension users not to have a desire to disconnect from our platform 🤗

It could make sense to include Supervisely in module names to make them easier to find in the module finder (Ctrl-F). Something like Supervisely Labeling and Supervisely Review.

I've updated the names

Installation failed for me on Windows. I had the current pandas version (2.2.1) installed by some other extensions. When I installed the extension and switched to Labeling Jobs Annotation module, it installed some things but then it just stopped. It did not restart Slicer.

The installation process has been updated

It would be also nice to make Supervisely compatible with current pandas version.

Unfortunately we can't update pandas to version 2.2.1 in our SDK right now. This will probably be done later. For now, the user will be informed about version conflicts during installation and will be prompted on the first activation of any extension module after (once) to restore previous version if there were conflicts.

In the Authentication tab, enter the required data -> I had trouble figuring out what to do here. Give simple instructions for registering, setting up a simple project, tell what server URL to use. By using the browser developer tools I figured out that app.supervisely.com is the server URL, but it should have been documented, or even better it should have been recommended by default.

I added a couple of lines of explanation and a link to a video on YouTube how our platform works. Working with our platform implies preliminary familiarization with it through documentation, blog posts, videos, etc. links to which you can see in the readme.

I added task list, etc. but the GUI in Slicer did not refresh. I disconnected and connected, which helped, but it was annoying that I had to enter the server, username, password again from scratch. It would be nice to have a refresh button or make things automatically refresh.

Yes, that was a drawback when there is only one item in the combobox. If you had several Teams, you could update the list of jobs by switching to another Team and back again. Now there are buttons to update the Job list and to Sync current Job. Implementing automatic updates would require constant API polling to retrieve user data, which is currently a bit at odds with the extension's concept of communicating with the platform only when the user explicitly requests it.
There is no need to update the Job list when the user has already opened a Job and is working on it. Perhaps in the future we will automate these moments completely.

I could not figure out the logic of how segmentations and segments are mapped. I tried to add tags, classes, objects in the web interface and segmentations and segments in Slicer and some segments were uploaded, but I still don't know what happened and why.

As for annotations and dynamic updating of classes and tags in Labeling Job - it depends on Labeling Job settings.
Now it is not supported in the extension, and also there is no two-way auto synchronization. It is worth understanding that if you directly change something in the project not through Labeling Job, it will not be synchronized. I have updated the documentation for this point. In future versions we will improve the synchronization mechanism.

Tags did not show up for me

Tags may not be displayed if they are not allowed in the Labeling Job settings. If it seems like a bug, please describe the actions that led to this situation.
☝️ The new behavior simply won't activate the Tags tab if there aren't any.

I could not download segmentation from the server into Slicer. Is this expected?

Could you please describe in more detail what exactly is not downloaded from the server, what is expected in Slicer and where. If we are talking about classes in the Supervisely project, their display in the segmentation list depends on the Labeling Job settings.

Team and Job combobox is set to be editable, This makes it more difficult to make a selection.

Changed them to non-editable

@lassoan
Copy link
Contributor

lassoan commented Apr 18, 2024

Thanks for the updates. I'm testing the installation now, if it works then this should be ready to merge.

Working with our platform implies preliminary familiarization with it through documentation, blog posts, videos, etc.

OK, that makes sense. I assumed that you would want to acquire new users from the Slicer community.

If you want to do that you would need to provide some minimal instructions on how to set up a very simple labeling project on Supervisely. These steps should not take more than a few minutes, because until people are convinced that they should use your system, they will just not invest significantly more time into learning it. If they are hooked then they may spend a lot of time and money, but the first steps should be extremely simple.

If you are new to Supervisely, please learn how our platform works and start at this point.
You can also find a lot of useful information on our Blog and on the Developer Portal.
In short, you need to organize Team and Workspace, create Project, import volume data, and create Labeling Job.

Video explanation of how it works

Just my example: I clicked on the https://docs.supervisely.com/ link. Nice overview, I have a general idea of what it does. 1 minute spent. Clicked next link: https://supervisely.com/blog/ - way too many entries, how would I know which one is relevant for me? I haven't clicked on any of them. 10 seconds spent, not useful. Last video: https://www.youtube.com/watch?v=YwNHbvyZL7Q. Looks relevant, but it is 10 minutes long. If I need to watch then follow the steps it would take me at least 30 minutes to do it. That's too long, I don't have the time right now. Maybe in the afternoon or tomorrow I'll get back. Of course, by then I'll have so many other things to do, so I'll actually probably never get back to this.

Anyway, all this is just free advice, helping you to reach Slicer users. If you want to get more feedback from me then I'll need more streamlined instructions on setting up a simple project in Supervisely. I don't read manuals or read blog posts, only when I'm already invested in some platform and very desperate to find information - and probably I'm not alone with this.

@lassoan
Copy link
Contributor

lassoan commented Apr 18, 2024

I've installed the latest Slicer Preview Release on Windows. Added ConnectToSupervisely modules, restarted Slicer, switched to one of the modules, asked a few confirmations for package installations and restarted Slicer. All OK, I would have preferred only one confirmation but two OK clicks, all within a couple of seconds is fine.

However, after restart I got this message:

Previously installed libraries were updated during the installation of the Supervisely package.

  • [urllib3] Previous version: 2.2.1, Current version: 2.2.0
    Do you want to restore the previous version of the libraries?
    Supervisely package will be uninstalled on restore. 3D Slicer will be restarted after the process is finished.

It has just asked me a minute ago if it can replace some packages and I consented. Why does it asks me again? It is also really hard to figure out what the correct answer is, because the buttons are "Yes" and "No". What do I press if I actually want to try Supervisely? A message like "Supervisely requires updating Python packages X, Y, Z" (more details about exactly what package versions etc. can be added if the user clicks "Show details" button - just set that in detailedText argument of the popup), with an OK button (update packages and proceed) and a Cancel button (do not show the module GUI just a message that Python package updates are needed).

Anyway, I clicked No and I got this error on the Python console:

Traceback (most recent call last):
  File "C:/D/SlicerConnectToSupervisely/ConnectToSupervisely/labelingJobsAnnotating.py", line 154, in enter
    slicer.util.reloadScriptedModule(activeModule)
  File "C:\Users\andra\AppData\Local\slicer.org\Slicer 5.7.0-2024-04-15\bin\Python\slicer\util.py", line 1422, in reloadScriptedModule
    filePath = modulePath(moduleName)
  File "C:\Users\andra\AppData\Local\slicer.org\Slicer 5.7.0-2024-04-15\bin\Python\slicer\util.py", line 1396, in modulePath
    return eval("slicer.modules.%s.path" % moduleName.lower())
  File "<string>", line 1, in <module>
AttributeError: module 'modules' has no attribute 'labelingjobsannotation'

After restart, labelingJobsAnnotating module initialization always takes a few seconds (while other modules are initialized in a fraction of a second). To avoid this, make sure you don't do any expensive operation (such as import of a large package) at startup. Instead you can import in the method you actually use it, import in your moduleLib at file scope, or use some lazy module initialization techniques.


So, I could not make it all work on a simple example within the time I could allocate to this, but the extension seems to be good enough to be added to the Extensions Index.

@lassoan lassoan removed the Status: Awaiting Response ⏳ Waiting for a response/more information label Apr 18, 2024
@lassoan lassoan merged commit 9762cae into Slicer:5.6 Apr 18, 2024
4 checks passed
@lassoan lassoan mentioned this pull request Apr 18, 2024
20 tasks
@GoldenAnpu
Copy link
Contributor Author

It has just asked me a minute ago if it can replace some packages and I consented.

The additional window after reloading 3D Slicer only appears when there were conflicts during installation. Despite the consent at the previous stage of installation, the user may change his mind for various reasons.
I have already realized that it would be better to put this feature as an additional button Restore in the module settings.

Anyway, I clicked No and I got this error on the Python console

This error is related to the renaming of a module whose data remained in the settings file after running the previous version of the extension.

@lassoan I am very pleased that you took the time to write such an extensive feedback, which will no doubt help to make our extension great. These are very valuable advices, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants