import --fetch-jars - Githubissues

manics commented 4 years ago

Thought I'd see how difficult it is to automatically fetch the jars....

$ rm -rf ~/omero/jars/

$ omero import test.fake
Downloading https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip

... import as normal...

Closes https://github.com/ome/omero-py/pull/161

manics commented 4 years ago

~~--exclude~~

For 5.6.1

joshmoore commented 4 years ago

Two pretty self-unsimilar options come to mind:

a more maven-like strategy (see jgo)
doing this without a flag (but perhaps a warning) to a temporary location; a flag could make it permanent: import --fetch-jars=$LOCATION

manics commented 4 years ago

OMERO.java is a 125 MB download, I don't think we should download that automatically

joshmoore commented 4 years ago

I'd bet most users would prefer for us to just do it, though maybe not repeatedly.

manics commented 4 years ago

Hard-coding the OMERO.java version could potentially be avoided by fetching the server version:

        c = self.ctx.conn(args)
        version = c.sf.getConfigService().getVersion()

Though this assumes OMERO.java version == OMERO.server version. It also requires the logic from https://github.com/ome/omego/blob/v0.7.0/omego/artifacts.py#L391 to figure out the build number.

joshmoore commented 4 years ago

cc: @sbesson

I'm still for having this somewhat transport rather than a solely being a new argument that needs calling.

manics commented 4 years ago

To make this more maintainable we could:

Move the omego artifacts plugin to a new omero cli plugin e.g. package-download
Use that plugin/library to download the OMERO client jars for the latest version (or perhaps the latest major version matching the OMERO-py version?)

manics commented 4 years ago

As requested I've made it automatically download the jars if required, the latest https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip will be fetched.

If you want to force a re-download, update or download a different version of the jars pass a version to the --fetch-jars argument:

$ rm -rf ~/omero/jars

$ omero import test.fake
Downloading https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip

<import as normal>

$ cat ~/omero/jars/OMERO.java.txt
OMERO.java-5.6.1-ice36-b225

$ omero import --fetch-jars 5.5
Downloading https://downloads.openmicroscopy.org/omero/5.5/OMERO.java.zip

$ cat ~/omero/jars/OMERO.java.txt
OMERO.java-5.5.1-ice36-b122

$ omero import --fetch-jars latest
Downloading https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip

$ cat ~/omero/jars/OMERO.java.txt
OMERO.java-5.6.1-ice36-b225

The available version strings are whatever https://downloads.openmicroscopy.org/omero/{version}/OMERO.java.zip redirects are in place on our downloads server.

manics commented 4 years ago

(Forgot to push the last commit before commenting)

manics commented 4 years ago

I had to mess around with the Ice build due to openssl issues. What I think is happening is that https://github.com/ome/zeroc-ice-py-manylinux/releases/download/0.2.0/zeroc_ice-3.6.5-cp36-cp36m-manylinux2010_x86_64.whl included a bundled openssl library which is somehow incompatible with Travis python.

https://github.com/ome/zeroc-ice-ubuntu1804/releases/download/0.3.0/zeroc_ice-3.6.5-cp36-cp36m-linux_x86_64.whl doesn't work, presumably because Travis Python 3.6 is different from the Ubuntu system Python 3.6.

A possible way around this is to build wheels specific to Travis, but for now I've changed this to use the default source package.

Note the reason https requests are being made is that although most tests in https://github.com/ome/omero-py/blob/master/test/unit/clitest/test_import.py that require the jars are disabled with @pytest.mark.skipif(OMERODIR is False, reason="Needs client dir") some of them aren't marked: https://github.com/ome/omero-py/blob/41f82a84a88a3a9153a2c48702f1e4c28fa86167/test/unit/clitest/test_import.py#L38 and are instead allowed to return a non-zero exit code if the jars are missing: https://github.com/ome/omero-py/blob/41f82a84a88a3a9153a2c48702f1e4c28fa86167/test/unit/clitest/test_import.py#L134-L138

$ omero import --advanced-help
Error: Could not find or load main class ome.formats.importer.cli.CommandLineImporter

With this PR the jars are now automatically downloaded.

If you're wondering whether the other import tests marked @pytest.mark.skipif(OMERODIR is False, reason="Needs client dir") should now pass, some of them do, but some of them are broken, suggesting we never run these unit tests with OMERODIR defined.

manics commented 4 years ago

See https://github.com/ome/omero-py/pull/231 (which includes the commits from this PR) for the fixed import tests. I can merge that PR into this one if you think it's appropriate.

joshmoore commented 4 years ago

I can merge that PR into this one if you think it's appropriate.

:+1:

Only other comment at this stage is whether or not omero import could print out the used version.

manics commented 4 years ago

I've:

rebased
added the 4 commits from https://github.com/ome/omero-py/pull/231
added omero.util.get_omero_user_cache_dir. Currently this is the same as get_omero_userdir but will change with https://github.com/ome/omero-py/issues/210#issuecomment-652919168. Since it's quite a big change I don't think we should do that in this PR.

manics commented 4 years ago

logback-cli.xml: I think it would make sense for it to be part of the OMERO.java zip, I don't know whether it's better to be inside a jar or outside. Since this PR now distinguishes between the downloaded OMERO.java and the default lib/client it should be easy to adjust the logback path depending on where the jars are found.

joshmoore commented 4 years ago

Ok. So if we update https://github.com/ome/openmicroscopy/blob/develop/build.xml#L302 we'll get logback-cli.xml into that directory and things should just magically start working... (Worth getting that location set now?)

Otherwise, :+1:

manics commented 4 years ago

After inserting an extra print to check:

$ git diff
diff --git a/src/omero/plugins/import.py b/src/omero/plugins/import.py
index d23312c1..c890f928 100644
--- a/src/omero/plugins/import.py
+++ b/src/omero/plugins/import.py
@@ -530,6 +530,7 @@ class ImportControl(BaseControl):
                     103, "No JAR files found under '%s'" % client_dir)

         logback = "-Dlogback.configurationFile=%s" % xml_file
+        print(logback)
         return classpath, logback

     def importer(self, args):

$ OMERO_USERDIR=/tmp/xx omero import --advanced-help
Using OMERO.java-5.6.1-ice36-b225
-Dlogback.configurationFile=/tmp/xx/jars/OMERO.java-5.6.1-ice36-b225/libs/logback-cli.xml
...

$ OMERODIR=/openmicroscopy/dist/ omero import --advanced-help
-Dlogback.configurationFile=/openmicroscopy/dist/etc/logback-cli.xml
...

joshmoore commented 4 years ago

Thanks again, @manics. I think this is going to make things much nicer for people since the move to OMERODIR. Merging.

(I'd add that as it gets usage there could well be feature requests for the likes of --list-jars, --del-jars=$OLD_VERSION, etc. which may even be best-placed in its own plugin.)

sbesson commented 4 years ago

Should we release the current state of master as omero-py 5.8.0 or would you like to include #233 ?

manics commented 4 years ago

It'd be nice to include #233, but that may need futher discussions, so I wouldn't say it's a blocker. Feel free to push to it if you want.

PRs merged since 5.7.1:

Using https://gist.github.com/manics/383b4481e1c3a193d224016b01a5d52a GH_TOKEN=$(git config github.token) GH_REPO=omero-py ../generate_release_notes/generate_release_notes.py v5.7.1 master --version v5.8.0

import --fetch-jars (#162)
fix logging attribute error (#226)
Add PyYAML as a dependency of omero-py (#228)
bin/omero entrypoint (#229)
hql.py: when querying masks, filter bytes field (#230)

https://github.com/ome/omero-py/pull/226 is the only bug fix so if that's not urgent I think we can wait a bit.

ome / omero-py

import --fetch-jars #162

PRs merged since 5.7.1: