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

Determine if an incremental-wrapping approach is possible or makes sense to make the server Python integration more Pythonic #1276

Closed
jmao-denver opened this issue Sep 13, 2021 · 2 comments
Assignees
Labels
feature request New feature or request triage
Milestone

Comments

@jmao-denver
Copy link
Contributor

The core class in DH/PY integration is Table, it is used both in output and input in many of the sub-modules. This means wrapping it to make it more Pythonic could be a breaking change for the depending sub-modules without rewriting everything. We need to find out for sure if that's the case.

under Epic #1263

@jmao-denver jmao-denver added feature request New feature or request triage labels Sep 13, 2021
@jmao-denver jmao-denver added this to the Sept 2021 milestone Sep 13, 2021
@jmao-denver jmao-denver self-assigned this Sep 13, 2021
@jmao-denver jmao-denver changed the title Determine if a piecemeal approach is possible to restructure the server side Python integration Determine if a piecemeal approach is possible to improving the server side Python integration Sep 13, 2021
@jmao-denver jmao-denver changed the title Determine if a piecemeal approach is possible to improving the server side Python integration Determine if an incremental-wrapping approach is possible or makes sense to make the server Python integration more Pythonic Sep 14, 2021
@jmao-denver
Copy link
Contributor Author

jmao-denver commented Sep 15, 2021

The stated goal of the epic is to make the server side Python integration (the deephaven package) much more Pythonic, thus providing a better experience to the Python developers. A parallel goal is to make it as seamless as possible for a client side Python script to run on the server and vice versa. The latter requires we have an identical or very similar design of the Python API on both the server and the client.

After having carefully examining the current implementation of Python integration on the server side, I have come to realize that the scope of the work is quite a bit bigger than I first thought. These are the main reasons behind that realization. Some of these reasons especially 1, 2, also argue strongly against a partial effort.

  1. io.deephaven.db.tables.Table is exposed at run-time via JPY, to wrap it in a hand-written Python class means that we need to wrap all the methods/functions that either return a Table object or receive one as an input. Otherwise, we will have two forms of ‘Table’ classes in the package, which is a nightmare none would want.
  2. The code style and naming conventions of the current Python package are of Java’s and are not consistent with that of the new Python client which follows the Python styles and conventions. It’d be best that we just wrap up all the Java classes that need to be accessible from Python and make them all consistent in terms of code style and naming convention.
  3. The documentation of the current Python package leaves a lot to be desired, as it is mostly generated from Java Doc, definitely not Pythonic with overloads and Java FQN all over the places. To make the documentation more Pythonic, we need to hand-craft proper Python docstrings and will probably have to wrap up a bit more Java classes than anticipated.
  4. A properly written Python wrapper of a Java class will also need to take care of data type mappings between Python and Java beyond what JPY offers.
  5. A JPY runtime exposed class makes avaiable all its public methods and members, a wrapper of it might need to do some curation work on those.
  6. The final hand-written Python package should also have a consistent Pythonic error handling strategy, this is something the current one has overlooked.
  7. Technical debt in the current Python package: misplaced functions, not-so-coherent content structure, and other questionable decisions means we need to do some good reorganization.
  8. A more comprehensive test suite would be nice for the new Python package. The current one can afford having minimal test cases because its directly-mapped nature and that the mapped engine Java classes should have adequate test coverage already. The new can still enjoy the same benefit but with additional Python code, it'd be best that we have a proper test suite.
  9. Current DHCC documentation/examples need to be checked against the rewritten package and updated accordingly.
  10. My lack of experience with some of the features in the current package beyond table operations will be a factor in how fast the project can move.

@jmao-denver
Copy link
Contributor Author

jmao-denver commented Sep 15, 2021

@rcaudy reviewed the comments and agreed with most of them. The parent epic will be my next big priority. As this is going to be a long project, we will need to break the epic into properly sized deliverables. More tickets are coming.

mofojed pushed a commit that referenced this issue Oct 25, 2023
Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.51.0

# [0.51.0](deephaven/web-client-ui@v0.50.0...v0.51.0) (2023-10-24)


### Bug Fixes

* Adjusted Monaco "white" colors ([#1594](deephaven/web-client-ui#1594)) ([c736708](deephaven/web-client-ui@c736708)), closes [#1592](deephaven/web-client-ui#1592)
* cap width of columns with long names ([#1574](deephaven/web-client-ui#1574)) ([876a6ac](deephaven/web-client-ui@876a6ac)), closes [#1276](deephaven/web-client-ui#1276)
* Enabled pointer capabilities for Firefox in Playwright ([#1589](deephaven/web-client-ui#1589)) ([f440a38](deephaven/web-client-ui@f440a38)), closes [#1588](deephaven/web-client-ui#1588)
* Remove @deephaven/app-utils from @deephaven/dashboard-core-plugins dependency list ([#1596](deephaven/web-client-ui#1596)) ([7b59763](deephaven/web-client-ui@7b59763)), closes [#1593](deephaven/web-client-ui#1593)
* Tab in console input triggers autocomplete instead of indent ([#1591](deephaven/web-client-ui#1591)) ([fbe1e70](deephaven/web-client-ui@fbe1e70))


### Features

* Theming - Spectrum Provider ([#1582](deephaven/web-client-ui#1582)) ([a4013c0](deephaven/web-client-ui@a4013c0)), closes [#1543](deephaven/web-client-ui#1543)
* Theming Iris Grid ([#1568](deephaven/web-client-ui#1568)) ([ed8f4b7](deephaven/web-client-ui@ed8f4b7))
* web-client-ui changes required for deephaven.ui ([#1567](deephaven/web-client-ui#1567)) ([94ab25c](deephaven/web-client-ui@94ab25c))
* Widget plugins ([#1564](deephaven/web-client-ui#1564)) ([94cc82c](deephaven/web-client-ui@94cc82c)), closes [#1455](deephaven/web-client-ui#1455) [#1167](deephaven/web-client-ui#1167)


### BREAKING CHANGES

- `usePlugins` and `PluginsContext` were moved from
`@deephaven/app-utils` to `@deephaven/plugin`.
- `useLoadTablePlugin` was moved from `@deephaven/app-utils` to
`@deephaven/dashboard-core-plugins`.
- `useConnection` and `ConnectionContext` were moved from
`@deephaven/app-utils` to `@deephaven/jsapi-components`.
- `DeephavenPluginModuleMap` was removed from `@deephaven/redux`. Use
`PluginModuleMap` from `@deephaven/plugin` instead.
* Enterprise will need ThemeProvider for the css
variables to be available





Co-authored-by: deephaven-internal <deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request triage
Projects
None yet
Development

No branches or pull requests

1 participant