Skip to content

update to QuPath 0.6.*#25

Merged
carlocastoldi merged 5 commits intoBIOP:mainfrom
carlocastoldi:qupath-0.6
Feb 19, 2025
Merged

update to QuPath 0.6.*#25
carlocastoldi merged 5 commits intoBIOP:mainfrom
carlocastoldi:qupath-0.6

Conversation

@carlocastoldi
Copy link
Copy Markdown
Collaborator

@carlocastoldi carlocastoldi commented Dec 6, 2024

with this PR i update gradle build to Kotlin following qupath/qupath-extension-template#21.
Additionally I adapted the code to avoid calling getServer() when it is avoidable.
Thanks to a new feature of QuPath 0.6.+ i worked with along with Pete, this allows the scripts to run in batch much faster.
In order to do it i had to use reflection on a quite stable QuPath interface (it is serialized into the retro-compatible save states).
Lastly, the check on the server is now done recursively. This is because the wrapping RotatedImageServer might also be wrapped into another TransformingImageServer.

I understand that this might seem a big PR. If you prefer I can slice it into multiples.

Fixes: #22

@NicoKiaru
Copy link
Copy Markdown
Member

Hi! I've added you as a maintainer of the project. Happy if you merge it.

I had a look and found nothing problematic imo. Too bad you had to resort to reflection. Maybe it's something fixable in the future ? Anyway it's good for now.

Just a quick question: it's working without updating the warpy dependency ? If yes, I'm happy.

@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

Hi! I've added you as a maintainer of the project.

Thank you a lot!
In the future I would like to avoid merging things without opening a PR, however. If that is ok for you, I'd like to keep your check as 4 eyes are better than 2 :D
Even just for a quick glance!

Happy if you merge it.

There are only a couple of things I'd like to have your full ACK:

  • by moving to kotlin, I had to move the publishing to Maven repositories. Do you reckon it's alright? I don't have much experience with it, sorry!
  • i updated the version from 0.3.* to 0.4.*, is that okay

Too bad you had to resort to reflection. Maybe it's something fixable in the future ? Anyway it's good for now.

Believe me, I've tried to avoid it but for now there is no other option. An likely nothing for the whole QuPath 0.6.* version.
The problem is that we should be able to read the rotation value (but also other transformation like Zoom, technically) from the metadata of the ImageServer builder.
However ImageServerBuilder interface is really... static in QuPath. Its structure is used in saving states and it makes every change a delicate operation.
Add to that the fact that very few projects (apart from ABBA) would use this feature as it's necessary only when you need to apply a transformation to a coordinate system in order to lazily import some annotations drawn by another program.

I don't see this being a short-term workaround :\

Just a quick question: it's working without updating the warpy dependency ? If yes, I'm happy.

Yes, as of now it's depending on warpy 0.3.1.
It's actually bundling the .jar extension with warpy dependecy included. On this regard, do you think we should update the installation instructions so that they don't require you to explicitly install warpy as well?
I also noticed that you are developing warpy 0.4.0 with QuPath 0.6.* support. Should I update to that version once it's available?

Thank you!

@NicoKiaru
Copy link
Copy Markdown
Member

I had to move the publishing to Maven repositories

What do you mean by that ? Everything was published to scijava maven before:

https://maven.scijava.org/#nexus-search;quick~qupath-extension-abba

Is it not published now ? Or is it published on central instead of scijava ?

i updated the version from 0.3.* to 0.4.*, is that okay

Sure; makes perfect sense since it's not compatible with QuPath 0.5 anymore.

I don't see this being a short-term workaround :\

Alright.

It's actually bundling the .jar extension with warpy dependecy included.

Ah, this is not great indeed. It's a fatjar ?

@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

I had to move the publishing to Maven repositories

What do you mean by that ? Everything was published to scijava maven before:
https://maven.scijava.org/#nexus-search;quick~qupath-extension-abba

Sorry, I explained myself badly. I meant that i moved and change the script to publish to scijava maven and I am not 100% sure it is correct until we'll run the CI/CD workflow here on github

Is it not published now ?

It is not, yes

It's actually bundling the .jar extension with warpy dependecy included.

Ah, this is not great indeed. It's a fatjar ?

Yes, warpy dependency is added with implementation("qupath.ext.warpy:qupath-extension-warpy:0.3.1"), not with shadow("qupath.ext.warpy:qupath-extension-warpy:0.3.1"). QuPath suggest to use the latter method just for qupath dependencies.
Actually, the current revision of build.gradle is also adding warpy with implementation() and not with shadow(). Doesn't that mean that, as of now, qupath-extension-abba is a fatjar?
Why do you prefer avoiding fatjars? Doesn't that increase the chance of bad behaviours due to mismatched versions of the extensions?

@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

carlocastoldi commented Jan 10, 2025

just as a clarification: the fatjar is built only with ./gradlew shadowJar. If we wished to keep warpy extension a forced dependency (thus breaking QuPath extension updater), then distributing the jar result of ./gradlew build is enough!

@NicoKiaru
Copy link
Copy Markdown
Member

thus breaking QuPath extension updater

Ah! Ok I don't know how to best handle this. But my opinion (is it good) is that people should install Warpy and the ABBA extension, a little bit like what happens with the Fiji update sites.

The other alternative is to merge Warpy and the ABBA repo, but I don't think it's a good idea because it breaks modularity.

For now let's keep the fatjar as what I understand is that it's easier to do it like that.

@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

I absolutely am in favour of keeping warpy extension a thing of its own. It may not only be useful to abba, but also to others as well.

That said i feel like shipping ABBA extension with the version of warpy with whom it was tested by the developer is the best idea. In order to do so, making the extension include the correct version seems the best sensible option.

It's true that if warpy extension gets an update, ABBA extension will have to be manually updated. This, however, is not a downside in my eyes as it requires us to check whether the dependency update breaks anything or not.

@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

I had to move the publishing to Maven repositories

What do you mean by that ? Everything was published to scijava maven before:
https://maven.scijava.org/#nexus-search;quick~qupath-extension-abba

Sorry, I explained myself badly. I meant that i moved and change the script to publish to scijava maven and I am not 100% sure it is correct until we'll run the CI/CD workflow here on github

now that 0.3.2 is released, I am no longer worried to break bad stuff on production branch hahaha
I have tested the critical bit regarding CI/CD with QuPath 0.6.0 on my repo and it seems to work fine.

I'm merging this

@carlocastoldi carlocastoldi merged commit adb6b30 into BIOP:main Feb 19, 2025
@carlocastoldi
Copy link
Copy Markdown
Collaborator Author

Too bad you had to resort to reflection. Maybe it's something fixable in the future ? Anyway it's good for now.

Believe me, I've tried to avoid it but for now there is no other option. And likely nothing for the whole QuPath 0.6.* version.
I don't see this being a short-term workaround :\

Alright.

Unfortunately, the possibility of resorting to a public API is out of question, for now: qupath/qupath#1715 (comment)

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.

prepare extension for QuPath 0.6.0

2 participants