-
Notifications
You must be signed in to change notification settings - Fork 80
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
Update readme #3722
Conversation
Co-authored-by: margaretkennedy <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
venv | ||
.venv | ||
*.egg-info |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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/
| Package | Version | OS | | ||
| ------- | ----------------------------- | ------------ | | ||
| Java | >=11, <20 | All | | ||
| Python | >= 3.8 | All | | ||
| Windows | 10 (OS build 20262 or higher) | Only Windows | | ||
| WSL | 2 | Only Windows | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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). |
There was a problem hiding this comment.
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.
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 | ||
``` |
There was a problem hiding this comment.
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?
- 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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. 🤷
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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.
Superceded by #4209 |
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.