Repository navigation
Add examples - #1
elijahpetty wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds example code demonstrating three Python packaging scenarios for Deephaven applications: a library-only package, a CLI-only package, and a combined package. The examples show how to structure Python packages using modern packaging standards with pyproject.toml, implement CLI tools with Click, and create reusable Deephaven query functions.
Changes:
- Added three complete example packages (my_dh_library, my_dh_cli, my_dh_toolkit) demonstrating different packaging approaches
- Included sample CSV data files for testing the examples
- Updated main README with comprehensive documentation on package structure, usage patterns, and troubleshooting
Reviewed changes
Copilot reviewed 34 out of 38 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| my_dh_library/* | Library-only package with reusable Deephaven query and utility functions |
| my_dh_cli/* | CLI-only package with command-line tools for CSV processing |
| my_dh_toolkit/* | Combined package with both library functions and CLI tools |
| data/* | Sample CSV files for testing the example packages |
| README.md | Comprehensive documentation covering all three packaging scenarios and usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Wording here is weird. Is "your" really the proper voice for this stuff? Assess the rest of the document for voice.
There was a problem hiding this comment.
Duplicates ./data/batch/*
There are a ton of these duplications. I'm not commenting further.
There was a problem hiding this comment.
Until these are fixed, the signal to noise is too bad for me to review further.
There was a problem hiding this comment.
🟡 Changes recommended
The code claims Python 3.8 support but uses list[str] annotations that break at runtime on Python 3.8 without postponed evaluation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/37 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
In reviewing this, I've had some confusion. This file looks like a concatenation of the prior files. I'm not sure if this is intentional, confusing, or just doesn't matter. I haven't read enough of the code to have a view. Just noting it here.
There was a problem hiding this comment.
In my quick scan of the code and the readme, I'm having a hard time figuring out what the story is. It seems like examples are doing multiple things at once, instead of each illustrating a single concept. The readme descriptions of the example then doesn't enlighten the case much.
Below is my chat with Copilot about this:
===
What is this readme trying to show? The illustrative examples seem garbled and unclear and like each does multipel things -- just making them confusing. Are they confusing?
===
Yes — looking at this diff, the README is trying to do too much in each section, and it does make the examples harder to follow. A few concrete problems:
- Each "Try the X package" section mixes multiple execution modes without clearly separating them
For example, the CLI-only section shows, in sequence:
installing the package
running it as an installed shell command (my-dh-query)
then also importing the underlying function directly in Python (from my_dh_cli.cli import my_dh_query)
These are two different usage patterns (CLI invocation vs. programmatic call) bolted together under one "Try it" header, with only a short transition sentence ("The underlying function is also importable...") separating them. A reader skimming for "how do I just run this from the terminal" has to wade through Python import code too.
- The combined package section is the worst offender
It shows, back-to-back: two CLI commands (my-dh-toolkit-query, my-dh-toolkit-process), then a full server-startup snippet, then library imports, then calling the same underlying functions two different ways (from my_dh_toolkit.queries import ... and from my_dh_toolkit import my_dh_query, batch_process). That's four different ways to do roughly two things, all in one code block region. It's demonstrating flexibility, but at the cost of a clear "here's the one obvious way to use this" path.
- Repeated boilerplate obscures the actual point of each example
The Server(port=10000, jvm_args=["-Xmx4g"]).start() snippet is repeated near-verbatim in almost every section. That's necessary context, but it dilutes the examples — the reader has to mentally filter out the same 3 lines every time to find what's actually different about that example.
- Structural/informational overlap
There's now a "Packaging scenarios" section (when to use library-only/CLI-only/combined) and a separate "Execution patterns" section (entry points vs. module execution) and per-package "Usage" snippets at the top and "Try the X package" walkthroughs further down. That's four different places explaining overlapping concepts (when/how to use each package type), which is a lot for a reader to reconcile into one mental model.
Net assessment: The content itself is accurate and reasonably complete, but the presentation conflates "here's the simplest way to use this" with "here's every way you could possibly use this." A cleaner version would pick one primary usage pattern per package type for the main walkthrough (e.g., just the CLI command) and move the "you can also import the underlying function" variant into a clearly separate, secondary callout — rather than interleaving them as equally-weighted steps.
If you'd like, I can draft a tightened version of one of these sections (e.g., the combined package walkthrough) to show the difference concretely.
There was a problem hiding this comment.
Copilot reviw for the repo was set at a low / cheap level. I increased the level and reran it.
There was a problem hiding this comment.
🟡 Changes recommended
Documentation includes non-working import and src-layout instructions, while batch CSV reads bypass intended CLI error handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
my_dh_toolkit/src/my_dh_toolkit/processor.py:33
- Unlike the single-file command, the batch command does not translate CSV read failures into
ClickException. An unreadable or malformed file therefore escapes the CLI as a raw exception/traceback instead of a contextual command error; wrap this read consistently withmy_dh_query.
README.md:232 - With the documented
src/layout, the project root does not placesrconsys.path, so this command fails withNo module named my_packageunless the package is installed (orPYTHONPATHis changed). Describe regular/editable installation as a prerequisite rather than claiming installation is unnecessary.
This issue also appears on line 320 of the same file.
setuptools-deployment.md:79
- The 4GB heap is not a default here; the preceding example explicitly requests it with
-Xmx4g. Calling it the default can mislead users sizing deployments, so describe it as this example's configured maximum heap.
setuptools-deployment.md:531 - The documented batch implementation omits the CSV-read error handling used by the single-file implementation, so users following this guide get a raw traceback for unreadable or malformed input. Keep the guide synchronized with the corrected package code by wrapping this call in
ClickException.
README.md:323
- This repeats the unsupported no-install workflow for a
src/-layout package. Running from the project root cannot discovermy_package; update the heading and benefits to reflect that a regular or editable installation is required.
**Run without installation:**
```shell
python -m my_package
- **Files reviewed:** 24/25 changed files
- **Comments generated:** 1
- **Review effort level:** Balanced
</details>
|
@copilot I want you to rereivew this changeset. This repo is intended to illustrate different ways that deephaven packages can be installed. Each example should be clear and provide an easy to understand story. The readme must be best practices user documentation showing users how to use the examples and modify the examples for their own use. Your prior review was of low quality and did not address any of these ultra important topics. |
…-end Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
Re-reviewed the full changeset against the three goals and fixed everything found, in bc71cf1. I validated by installing all three packages and running every documented command and snippet end-to-end against a live Deephaven server. Findings and fixes:
|
…ync guide Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The batch processor can attempt to read .csv-named directories, and the guide repeats the faulty implementation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (4)
| - In a package that defines commands, keep `__init__.py` free of imports that reach `deephaven`. Expose library functions from submodules (`my_dh_toolkit.queries`, `my_dh_toolkit.utils`) instead. | ||
| - Inside command modules, import `deephaven` and any library submodules lazily, inside the function that runs after the server has started. | ||
| - A library-only package such as `my_dh_library` can safely re-export its functions from `__init__.py`. It has no commands, so it is only ever imported after a server is running. |
There was a problem hiding this comment.
I'm not convinced that these rules are correct. For example, submodules can also import deephaven and get into trouble.
Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
- Fix duplicated `server = server =` assignment in root README snippets - Rename my_dh_cli entry function `app` to `main` to match my_dh_toolkit - Explain deferred deephaven imports in my_dh_cli (code comment + README) - Split the long my_dh_toolkit __init__.py explanation into short bullets - Remove redundant toolkit summary under the pattern table - Note that toolkit's queries.py/utils.py are deliberate copies Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
@copilot full review |
Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
Full review complete. I found one user-facing issue and fixed it in 333eac6: all three CLI entry points now convert Deephaven server startup failures into |
| f"Available columns: {', '.join(column_names)}" | ||
| ) | ||
|
|
||
| result = source.update(formulas=["DoubleScore = Score * 2"]) |
| except ValueError as e: | ||
| raise click.ClickException(f"File '{csv_file.name}': {e}") | ||
|
|
||
| processed = add_computed_columns(table) |
| except ValueError as e: | ||
| raise click.ClickException(f"File '{input_path.name}': {e}") | ||
|
|
||
| result = add_computed_columns(source) |
The accompanying doc will be in a deephaven-core PR.