Depend on public jpy#2583
Conversation
devinrsmith
added
clean up
release blocker
| version=__normalized_version__, | ||
| description='Deephaven Embedded Server Python Package', | ||
| long_description=README, | ||
| long_description_content_type='text/markdown', |
There was a problem hiding this comment.
long_description_content_type is necessary for publication.
| test_loader='unittest:TestLoader', | ||
| classifiers=[ | ||
| 'Development Status :: 2 - Pre-Alpha', | ||
| 'Intended Audience :: Developers, Data Scientists', |
There was a problem hiding this comment.
Data Scientists is improper classifier.
| version=__normalized_version__, | ||
| description='Deephaven Engine Python Package', | ||
| long_description=README, | ||
| long_description_content_type='text/markdown', |
There was a problem hiding this comment.
long_description_content_type is necessary for publication
| test_loader='unittest:TestLoader', | ||
| classifiers=[ | ||
| 'Development Status :: 2 - Pre-Alpha', | ||
| 'Intended Audience :: Developers, Data Scientists', |
There was a problem hiding this comment.
Data Scientists is invalid classifier.
| repositories { | ||
| mavenCentral() | ||
|
|
||
| // Uncomment the following if doing local development work that depends on SNAPSHOTs |
There was a problem hiding this comment.
probably should remove this? or if helpful generally, guard it behind a -P flag, and add mavenLocal for any locally installed builds?
personally i'd prefer it not be there, since it could have surprising other effects, but i can see either way
There was a problem hiding this comment.
I don't like the idea of -P guards in buildSrc. Maybe we've already done that elsewhere? That said, I think there are some development workflows we may want to make easier now that jpy is external (and deephaven-csv). I'll remove for now.
| google-auth-oauthlib==0.4.6 | ||
| google-pasta==0.2.0 | ||
| grpcio==1.46.3 | ||
| grpcio==1.47.0 |
There was a problem hiding this comment.
why advance this past the 1.46 that the server (and java client) presently uses?
There was a problem hiding this comment.
that said, make sure all py modules get updated accordingly - at a glance i see sphinx (for our docs) is still using 1.46.0, not changed in this diff
There was a problem hiding this comment.
This is a requirement inherited from tensorflow - the dependencies regularly get updated. As part of update adding jpy as a proper base image dependency, some of the transitive dependencies got updated as well. deephaven/deephaven-base-images#29
| jpy.VerboseExceptions.enabled = True | ||
|
|
||
|
|
||
| def _expandLinks(dirname): |
There was a problem hiding this comment.
these specifically existed in case a caller was specifying wildcard paths to their own extra jars
There was a problem hiding this comment.
Note: java has the mechanisms accepting wildcards natively - I don't think we should try to do the expanding ourselves if we re-introduce custom classpaths.
There was a problem hiding this comment.
Note that while java does have this mechanism from the command line, it does not work when starting the jvm as we do (at least not in my testing, which IIRC was java 11).
| server.run(); | ||
| checkGlobals(scriptSession.get(), null); | ||
| System.out.println("Server started on port " + server.server().getPort()); | ||
| System.out.println("Server started on: " |
There was a problem hiding this comment.
Wrong patch? Also present in conventional server startup, not really part of this diff
There was a problem hiding this comment.
Yes - I'll pull out this for separate consideration.
| # Expand the classpath, so a user can resolve wildcards | ||
| expanded_classpath = list(itertools.chain.from_iterable(iglob(e, recursive=True) for e in extra_classpath)) |
There was a problem hiding this comment.
@jmao-denver - can you give this a lookover - I think it replaces the previous functionality. I've tested it out, and it seems to work.
| @@ -3,7 +3,7 @@ plugins { | |||
| } | |||
|
|
|||
| dependencies { | |||
| pythonWheel project(path: ':deephaven-jpy', targetConfiguration: 'pythonWheel') | |||
|
|
|||
niloc132
left a comment
niloc132
left a comment
There was a problem hiding this comment.
I think you might still be working on the py 3.9 bug - but one final request either in this ticket or in a post-release followup, that we broaden the testing a bit, and get CI to confirm on multiple py versions that we can at least start up the server, run something simple, and shut it down safely.
|
Follow-up: #2593 |
|
Follow-up: #2592 |
4 participants
No description provided.