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

fix: Rename directory containing proto files #6379

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

niloc132
Copy link
Member

In order to make directory structures usable for downstream projects seeking to add new services or messages that depend on our own proto files, ensure that we name/package consistently for all languages.

This may be a breaking change for some downstream consumers, but these changes should prevent needing to make future changes or add additional build steps. Two places this may impact applications:

  • The Python client proto package has been renamed from pydeephaven.proto to deephaven_core.proto. The old import will continue to work in this release, but will warn the first time it is referenced.
  • The deephaven-proto-backplane-grpc.jar still contains the .proto files, but they are located in the deephaven_core/proto directory instead of at the root of the jar.

Fixes #6376

@niloc132 niloc132 added jsapi grpc NoDocumentationNeeded cpp api release blocker A bug/behavior that puts is below the "good enough" threshold to release. python-client ReleaseNotesNeeded Release notes are needed go-client labels Nov 15, 2024
@niloc132 niloc132 added this to the 0.37.0 milestone Nov 15, 2024
@niloc132
Copy link
Member Author

Technically breaking changes in the deephaven-proto-backplane-grpc jar:

$ unzip -l proto/proto-backplane-grpc/build/libs/deephaven-proto-backplane-grpc-0.37.0-SNAPSHOT.jar

...
        0  1980-02-01 00:00   deephaven_core/
        0  1980-02-01 00:00   deephaven_core/proto/
     1467  1980-02-01 00:00   deephaven_core/proto/application.proto
     1294  1980-02-01 00:00   deephaven_core/proto/config.proto
    17768  1980-02-01 00:00   deephaven_core/proto/console.proto
    11218  1980-02-01 00:00   deephaven_core/proto/hierarchicaltable.proto
     1260  1980-02-01 00:00   deephaven_core/proto/inputtable.proto
     7082  1980-02-01 00:00   deephaven_core/proto/object.proto
     3707  1980-02-01 00:00   deephaven_core/proto/partitionedtable.proto
     9090  1980-02-01 00:00   deephaven_core/proto/session.proto
     4729  1980-02-01 00:00   deephaven_core/proto/storage.proto
    40195  1980-02-01 00:00   deephaven_core/proto/table.proto
      944  1980-02-01 00:00   deephaven_core/proto/ticket.proto
     8064  1980-02-01 00:00   META-INF/LICENSE
      527  1980-02-01 00:00   META-INF/NOTICE
---------                     -------
 11033853                     1284 files

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Couple small things, look great.

py/client/build.gradle Outdated Show resolved Hide resolved
py/client/build.gradle Outdated Show resolved Hide resolved
py/client/build.gradle Show resolved Hide resolved
@@ -37,7 +37,7 @@ def _compute_version():
description='The Deephaven Python Client',
long_description=_get_readme(),
long_description_content_type="text/markdown",
packages=find_packages(exclude=("tests",)),
packages=find_namespace_packages(exclude=("tests","examples","docs","build"),),
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I see docs here, but docs/source/conf.py is not being excluded, I'm guessing b/c it has a .py extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently the exclude isn't recursive, will investigate.

Copy link
Member

Choose a reason for hiding this comment

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

The directory structure doesn't contain __init__.py files, so I wonder if it is not treating it as a package and therefore not excluding. Just a wild guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we only want it to be a namespace, not a module - if we required that there be a __init__.py file, that could conflict with other wheels from contributing to it (at least without stomping on each other).

py/client/setup.py Outdated Show resolved Hide resolved
py/client/pydeephaven/proto/__init__.py Outdated Show resolved Hide resolved
@niloc132
Copy link
Member Author

Current test failure is a packaging-for-the-test issue:

======================================================================
ERROR [0.000s]: setUpClass (tests.test_barrage.BarrageTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/python/tests/test_barrage.py", line 30, in setUpClass
    cls.ensure_server_running()
  File "/python/tests/test_barrage.py", line 39, in ensure_server_running
    from pydeephaven.session import Session
  File "/opt/deephaven/venv/lib/python3.10/site-packages/pydeephaven/__init__.py", line 28, in <module>
    from .session import Session
  File "/opt/deephaven/venv/lib/python3.10/site-packages/pydeephaven/session.py", line 21, in <module>
    from pydeephaven._app_service import AppService
  File "/opt/deephaven/venv/lib/python3.10/site-packages/pydeephaven/_app_service.py", line 7, in <module>
    from deephaven_core.proto import application_pb2_grpc, application_pb2
ModuleNotFoundError: No module named 'deephaven_core'

chipkent
chipkent previously approved these changes Nov 15, 2024
nbauernfeind
nbauernfeind previously approved these changes Nov 18, 2024
Copy link
Member

@nbauernfeind nbauernfeind left a comment

Choose a reason for hiding this comment

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

LGTM

devinrsmith
devinrsmith previously approved these changes Nov 18, 2024
rcaudy
rcaudy previously approved these changes Nov 18, 2024
@niloc132 niloc132 enabled auto-merge (squash) November 20, 2024 21:34
@niloc132 niloc132 merged commit 8df9655 into deephaven:main Nov 20, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cpp api go-client grpc jsapi NoDocumentationNeeded python-client release blocker A bug/behavior that puts is below the "good enough" threshold to release. ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python client proto output makes it difficult to compile new protos for use in clients
6 participants