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

Bump server to 5.6.11 and incl ZarrReader #420

Closed
wants to merge 2 commits into from

Conversation

dominikl
Copy link
Member

This reverts commit 3876a0f.
Currently testing this by spinning up pilot-idrngff

@will-moore
Copy link
Member

From @sbesson:

- src: ome.omero_server
version: 6.0.0
probably needs to be upgraded to 6.1.0 as the role should install [blosc] as a requirement

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

As discussed today, the plan for prod122 is to bump OMERO.server to the just released 5.6.11 version /cc @jburel @francesw

ansible/group_vars/omero-hosts.yml Outdated Show resolved Hide resolved
Co-authored-by: Sébastien Besson <sbesson@glencoesoftware.com>
@dominikl dominikl changed the title Revert "Revert OMERO.server upgrade for prod120" Bump server to 5.6.11 and incl ZarrReader May 20, 2024
@sbesson sbesson self-requested a review May 21, 2024 07:55
@sbesson
Copy link
Member

sbesson commented May 21, 2024

Deployed on test122

@will-moore
Copy link
Member

See https://github.com/ome/ZarrReader/blob/main/CHANGELOG.md

  • Probably best to bump ZarrReader to 0.5.1 (Bio-Formats 7.3.0).
  • Not sure where to specify the bump of Bio-Formats 7.3.0 in IDR/deployment?

@sbesson
Copy link
Member

sbesson commented Jun 11, 2024

Not sure where to specify the bump of Bio-Formats 7.3.0 in IDR/deployment?

This PR is upgrading OMERO.server to version 5.6.11 which should ship Bio-Formats 7.3.0 by default so I don't think anything else is required.

Probably best to bump ZarrReader to 0.5.1 (Bio-Formats 7.3.0).

Agreed, I would propose to declare the version of ZarrReader as a variable in ansible/group_vars/omero-hosts.yml and consuming it in idr-omero.yml and idr-omeroreadonly.yml to ensure the same JAR is deployed everywhere.

@will-moore
Copy link
Member

@khaledk2 - As discussed above, we will be using ZarrReader 0.5.1 (Bio-Formats 7.3.0) on idr-testing.
I'm going to manually update the OMEZarrReader.jar to the released 0.5.1 now, and check that it works with the memo files that were generated with the pre-release builds.

@dominikl - Do you want to look at Seb's suggestion above:?

I would propose to declare the version of ZarrReader as a variable in ansible/group_vars/omero-hosts.yml and consuming it in idr-omero.yml and idr-omeroreadonly.yml to ensure the same JAR is deployed everywhere.

@dgault
Copy link
Contributor

dgault commented Jun 17, 2024

Following on from the Slack discussion, updating ZarrReader to 0.5.1 will mean we will also need to include the AWS SDK jar from here.

We have 2 options:
1 - Update this PR to include the AWS dependency in the same way as ZarrReader is currently added
2 - I can build and deploy an uber jar for ZarrReader that includes all the dependencies and we update to use that

My preference would be to go for option 1 in this case, but in the short term I am happy to deploy an uber jar if that is easiest.

@sbesson
Copy link
Member

sbesson commented Jun 17, 2024

Running mvn dependency:tree, the AWS SDK brings the following dependencies

[INFO] +- com.amazonaws:aws-java-sdk-s3:jar:1.12.659:compile
[INFO] |  +- com.amazonaws:aws-java-sdk-kms:jar:1.12.659:compile
[INFO] |  +- com.amazonaws:aws-java-sdk-core:jar:1.12.659:compile
[INFO] |  |  +- commons-codec:commons-codec:jar:1.15:compile
[INFO] |  |  +- org.apache.httpcomponents:httpclient:jar:4.5.13:compile
[INFO] |  |  |  \- org.apache.httpcomponents:httpcore:jar:4.4.13:compile
[INFO] |  |  \- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:jar:2.12.6:compile
[INFO] |  \- com.amazonaws:jmespath-java:jar:1.12.659:compile
[INFO] \- commons-logging:commons-logging:jar:1.2:compile

Some of these e.g. commons-codec/commons-logging will already be included through other server dependencies although the versions probably need to be checked. Otherwise, all these JARs will need to be deployed individually (assuming from the OME artifactory as well) under lib/client and lib/server

@will-moore
Copy link
Member

I deployed all the dependency jars manually on idr-testing last time around: IDR/idr-metadata#675 (comment) but didn't include aws

@khaledk2
Copy link
Contributor

khaledk2 commented Jun 17, 2024

I have built the Micro-service. The packages, that Seb mentioned, are added by other Micro-service dependencies.

I have checked the lib folder and the dependencies versions which Seb mentioned are the same except jackson-dataformat-cbor.

The version of jackson-dataformat-cbor.jar is 2.14.2 but Seb mentioned it is 2.12.6. I am not sure if that may cause any issue

I have used this OMEZarrReader version: in the build
https://artifacts.openmicroscopy.org/artifactory/ome.releases/ome/OMEZarrReader/0.5.1/OMEZarrReader-0.5.1.jar

@sbesson
Copy link
Member

sbesson commented Jun 18, 2024

Mid-term, I assume the goal is to resolve these dependencies using a build system and ship them with OMERO.server. As an intermediate solution, either option proposed in #420 (comment) should work although using the individual JARs should be closer to the

The version of jackson-dataformat-cbor.jar is 2.14.2 but Seb mentioned it is 2.12.6. I am not sure if that may cause any issue

2.12.6 is the version that is pulled from the aws-java-sdk-s3 dependency but the same component might be declared with a different version by another dependency. In the micro-services, ./gradlew dependencies will tell you what the dependency tree looks like

@will-moore
Copy link
Member

I'm trying to work out what changes are needed to this PR to include the list of jars at #420 (comment)
From that list I think I can guess the jar names , e.g. com.amazonaws:aws-java-sdk-s3:jar but not the URL.
And do these get added as Override lib/server JARs tasks like this for each of the 8 jars (not including logging)?

diff --git a/ansible/idr-omero.yml b/ansible/idr-omero.yml
index 1e1c307..926843a 100644
--- a/ansible/idr-omero.yml
+++ b/ansible/idr-omero.yml
@@ -91,6 +91,22 @@
         force: yes
       notify: restart omero-server
 
+    - name: Override lib/server JARs
+      become: yes
+      get_url:
+        url: "com.amazonaws:aws-java-sdk-s3:jar"
+        dest: "{{ omero_common_basedir }}/server/OMERO.server/lib/server/aws-java-sdk-s3.jar"
+        force: yes
+      notify: restart omero-server
+
+    - name: Override lib/client JARs
+      become: yes
+      get_url:
+        url: "com.amazonaws:aws-java-sdk-s3:jar"
+        dest: "{{ omero_common_basedir }}/server/OMERO.server/lib/client/aws-java-sdk-s3.jar"
+        force: yes
+      notify: restart omero-server
+
     - name: Remove OMERO scripts
       become: yes
       file:

@sbesson
Copy link
Member

sbesson commented Jun 19, 2024

Yes all required JARs need to be under lib/server minimally and lib/client if they are expected to be used e.g. for imports.

From that list I think I can guess the jar names , e.g. com.amazonaws:aws-java-sdk-s3:jar but not the URL.

Minimally, all these dependencies should be on Maven Central i.e. https://repo1.maven.org/maven2/. They might be mirrored under the OME artifactory as well @jburel?

If we can use the OME artifactory, it might be worth re-using the existing logic i.e. define a list of (group, name, version) coordinates in the group variables and have a single task that loops over these dependencies, retrieves "{{ artifactory_baseurl }}/{{ item.group }}/{{ item.name }}/{{ item.version }}/{{ item.name }}-{{ item.version }}.jar and puts it under the right place

@dominikl
Copy link
Member Author

Replaced by #427

@dominikl dominikl closed this Jun 20, 2024
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.

5 participants