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

Update readme #3722

Closed
wants to merge 4 commits into from
Closed

Update readme #3722

wants to merge 4 commits into from

Conversation

mattrunyon
Copy link
Contributor

This at least adds some info about development scripts/workflow for Python/Java. Also removes the bulk of the main readme which was a copy our docs quickstart guide

There could still be more improvements such as adding info on how to do testing

Added placeholders for things I know have to be manually generated if there are protobuf changes.

proto/README.md Outdated Show resolved Hide resolved
proto/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
py/README.md Outdated Show resolved Hide resolved
jmao-denver
jmao-denver previously approved these changes Apr 24, 2023
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM


## JS API

Needs info on how to generate
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that this and the tests section in the py file still need info.

Comment on lines +50 to +52
venv
.venv
*.egg-info
Copy link
Member

Choose a reason for hiding this comment

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

Why were these removed? I think the venv is there to handle venvs that pycharm creates, and the egg-info is for python files that are not checked in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were added. And anything in .gitignore without a leading or middle slash will match at any level. So it is effectively the same as **/venv/** or "ignore any folder at any depth called venv". I could actually remove the pyclient/venv since it will match under this new item

Comment on lines -37 to +39
Developers interested in tinkering with and modifying source code should build from the source code. For detailed instructions on how to do this, see [Build and launch Deephaven](https://deephaven.io/core/docs/how-to-guides/launch-build).
Developers interested in modifying source code should read on for development instructions.
Copy link
Member

Choose a reason for hiding this comment

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

The link to the build docs on the website should not have been lost.
https://deephaven.io/core/docs/how-to-guides/launch-build/

Comment on lines +47 to +52
| Package | Version | OS |
| ------- | ----------------------------- | ------------ |
| Java | >=11, <20 | All |
| Python | >= 3.8 | All |
| Windows | 10 (OS build 20262 or higher) | Only Windows |
| WSL | 2 | Only Windows |
Copy link
Member

Choose a reason for hiding this comment

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

This dependency list does not match what is on the website.
https://deephaven.io/core/docs/how-to-guides/launch-build/
Is python a hard requirement? If not, that is not represented in either place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python isn't a hard requirement. Docker isn't either technically I think.

I'm also not sure the Java version is entirely correct here or the website. I think gradle requires a more specific version to run, but not 100% sure (I had some issues in intellij b/c I had my gradle version set too new)

@@ -2,6 +2,8 @@

![Deephaven Data Labs Logo](docs/images/Deephaven_GH_Logo.svg)

[Quick start for Docker user guide](https://deephaven.io/core/docs/tutorials/quickstart)
Copy link
Member

Choose a reason for hiding this comment

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

We should also reference the quick start for pip.
https://deephaven.io/core/docs/tutorials/quickstart-pip/

@@ -32,269 +34,51 @@ which functions as the data backbone for prominent hedge funds, banks, and finan

## Run Deephaven

This section is a quick start guide for running Deephaven from pre-built images. Almost all users will want to run Deephaven using pre-built images. It is the easiest way to deploy. For detailed instructions, see [Launch Deephaven from pre-built images](https://deephaven.io/core/docs/tutorials/quickstart).
If you want to use Deephaven, see our [Quick start for Docker guide](https://deephaven.io/core/docs/tutorials/quickstart).
Copy link
Member

Choose a reason for hiding this comment

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

We should also be directing people to https://deephaven.io/core/docs/tutorials/quickstart-pip/. We need to provide very obvious flight paths for docker vs pip users. This document is so lean that there is one flight path for end users.

Comment on lines +9 to +15
For any of the options below, you should first set up a virtual environment in Python. Once created, you must activate the virtual environment every time you open a shell.

```sh
python3 -m venv .venv # Create. Only needed once
source .venv/bin/activate # Activate the virtual environment
deactivate # Exit virtual environment when done
```
Copy link
Member

Choose a reason for hiding this comment

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

Where does this need to be set up?

Comment on lines +19 to +28
- This is not an exhaustive guide to managing Python environments.
- Depending on your OS and how your PATH is set up, you may need to use `python3`, or a path to the explicit Python version you want to use.
- You may set up a "permanent" virtual environment location.
- You'll need to re-install the wheel whenever you make Python code changes that affect the wheel.
- `pip` can be a pain if you are trying to (re-)install a wheel with the same version number as before.
- A `pip install --force-reinstall --no-deps "py/server/build/wheel/deephaven_core-<version>-py3-none-any.whl[autocomplete]"` may do the trick.
- You can install other Python packages in your venv using `pip install <some-other-package>`.
- You can set up multiple virtual environments and switch between them as necessary using `source /path/to/other-venv/bin/activate`.
- You can de-activate the virtual environment by running `deactivate`.
- You can use the `VIRTUAL_ENV` environment variable instead of sourcing/activating virtual environments: `VIRTUAL_ENV=/my/venv ./gradlew server-jetty-app:run`.
Copy link
Member

Choose a reason for hiding this comment

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

This is not thorough enough for an outside user to make progress.

Copy link
Member

Choose a reason for hiding this comment

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

This document is ultra fragmented and there are not clear thought flows for users to follow. This is more core dump than outside-usable instructions.

Copy link
Member

Choose a reason for hiding this comment

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

This is ultra stripped down. It needs some more background for outsiders.

- [Python](./py/README.md)
- [Web](./web/client-ui/README.md)
- [Go](./go/README.md)
- [C++](./cpp-client/README.md)
Copy link
Member

Choose a reason for hiding this comment

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

Needs an R link.

- You may set up a "permanent" virtual environment location.
- You'll need to re-install the wheel whenever you make Python code changes that affect the wheel.
- `pip` can be a pain if you are trying to (re-)install a wheel with the same version number as before.
- A `pip install --force-reinstall --no-deps "py/server/build/wheel/deephaven_core-<version>-py3-none-any.whl[autocomplete]"` may do the trick.
Copy link
Member

Choose a reason for hiding this comment

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

Don't we also need to install the py/server-embedded/.../deephaven_server wheel? I've been doing that... but maybe I don't need to. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found it unnecessary if you're not using the pip installed version of DH (DHaaL I think we called it?)

This section was just moved from the jetty-app readme b/c it didn't make sense there IMO, but it does look like it was just a brain dump to get the info on the page

- You can install other Python packages in your venv using `pip install <some-other-package>`.
- You can set up multiple virtual environments and switch between them as necessary using `source /path/to/other-venv/bin/activate`.
- You can de-activate the virtual environment by running `deactivate`.
- You can use the `VIRTUAL_ENV` environment variable instead of sourcing/activating virtual environments: `VIRTUAL_ENV=/my/venv ./gradlew server-jetty-app:run`.
Copy link
Member

Choose a reason for hiding this comment

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

We should also detail debugging steps somewhere. Just adding -Pdebug allows you to attach a remote debugger and set breakpoints.

@devinrsmith devinrsmith mentioned this pull request Jul 6, 2023
@jjbrosnan jjbrosnan mentioned this pull request Jul 20, 2023
@mofojed
Copy link
Member

mofojed commented Jul 24, 2023

Superceded by #4209
@mattrunyon will make sure the changes from @jjbrosnan incorporate the developer development instructions

@mofojed mofojed closed this Jul 24, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants