-
Notifications
You must be signed in to change notification settings - Fork 33
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
import --fetch-jars #162
import --fetch-jars #162
Conversation
For |
Two pretty self-unsimilar options come to mind:
|
OMERO.java is a 125 MB download, I don't think we should download that automatically |
I'd bet most users would prefer for us to just do it, though maybe not repeatedly. |
Hard-coding the OMERO.java version could potentially be avoided by fetching the server version:
Though this assumes |
cc: @sbesson I'm still for having this somewhat transport rather than a solely being a new argument that needs calling. |
To make this more maintainable we could:
|
cbe1ca8
to
d988b3f
Compare
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
The available version strings are whatever |
(Forgot to push the last commit before commenting) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the auto-download 👍 The re-fetching based on versions also looks good. Has a jenv
/rbenv
vibe to it. Only other thought I had was whether or not the choice could be via symlink, but that would be less fun on Windows, I assume.
src/omero/plugins/import.py
Outdated
@@ -517,6 +542,43 @@ def importer(self, args): | |||
else: | |||
self.do_import(command_args, xargs) | |||
|
|||
def _userdir_jars(self, parentonly=False): | |||
user_jars = get_omero_userdir() / 'jars' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections to this unless we foresee moving away from ~/omero
soonish. If so, then perhaps something like user_cache_dir()/omero would be safer (so people don't have to re-download).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair point, but I think having two different dirs for storing user state is worse than re-downloading. If there's a concrete suggestion for the location of user_cache_dir()
shouldn't we modify get_omero_userdir()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My proposal for moving forward would be to place an omero
subdirectories into existing directories like .config
, .cache
. If that's controversial at all, let's not block this PR.
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 omero-py/test/unit/clitest/test_import.py Line 38 in 41f82a8
and are instead allowed to return a non-zero exit code if the jars are missing: omero-py/test/unit/clitest/test_import.py Lines 134 to 138 in 41f82a8
With this PR the jars are now automatically downloaded. If you're wondering whether the other import tests marked |
See #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. |
👍 Only other comment at this stage is whether or not |
For windows compatiblity a text file is used on all platforms in lieu of a symlink
Currently this is the same as get_omero_userdir but this will change soon as part of ome#210 (comment)
I've:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Two small code issues. The only other thing I notice is that when using the downloaded files that debugging is skewed since logback-cli.xml
is missing (perhaps related to some of your test issues?)
Options I can think of:
- move logback-cli.xml to one of the jars
- download it separately
- bundle it into python (?!)
if not jars_dir.exists(): | ||
jars_dir.mkdir() | ||
with urlopen(omero_java_zip) as resp: | ||
with ZipFile(BytesIO(resp.read())) as zipfile: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may fall more under #217, but some feedback of the download progress at least in an interactive terminal will probably prevent consternation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could look into TQDM https://github.com/tqdm/tqdm
But that's another dependency- should we split omero-cli out of omero-py to avoid installing UI dependencies before going further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I imagine that's one we put on the backburner.
Print out OMERO.java version if using auto-downloaded jars
|
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, 👍 |
After inserting an extra $ 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):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested using a fresh omero-py
virtual environment
$ venv/bin/omero import test.fake -f
Downloading https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip
Using OMERO.java-5.6.1-ice36-b225
...
$ rm -rf ~/omero/jars/
$ venv/bin/omero import test.fake -f
Downloading https://downloads.openmicroscopy.org/omero/latest/OMERO.java.zip
Using OMERO.java-5.6.1-ice36-b225
...
$ rm -rf ~/omero/jars/
$ venv/bin/omero import --fetch-jars 5.6
Downloading https://downloads.openmicroscopy.org/omero/5.6/OMERO.java.zip
$ venv/bin/omero import test.fake -f
Using OMERO.java-5.6.1-ice36-b225
...
$ venv/bin/omero import --fetch-jars 5.4
Downloading https://downloads.openmicroscopy.org/omero/5.4/OMERO.java.zip
$ venv/bin/omero import test.fake -f
Using OMERO.java-5.4.10-ice36-b105
...
The feature is very useful and the newly introduced API makes sense.
Are we considering a release omero-py 5.8.0
with this PR included? Shoudl this coordinated with the upcoming release of OMERO.java including the logging configuration file?
Co-authored-by: Sébastien Besson <seb.besson@gmail.com>
Thanks again, @manics. I think this is going to make things much nicer for people since the move to (I'd add that as it gets usage there could well be feature requests for the likes of |
Should we release the current state of |
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
#226 is the only bug fix so if that's not urgent I think we can wait a bit. |
return None, omero_java_txt | ||
|
||
def download_omero_java(self, version): | ||
omero_java_zip = OMERO_JAVA_ZIP.format(version=version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurs to me for the testing of ome/openmicroscopy#6255 that version
should likely have been used directly if it started with http(s)://
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see: #303
Thought I'd see how difficult it is to automatically fetch the jars....
Closes #161