-
Notifications
You must be signed in to change notification settings - Fork 88
Standardize Environment Directory Structure #145 #160
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @rycerzes! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
@zkwentz @burtenshaw @init27 @jspisak this is the PR for #145 |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Amazing @rycerzes I took a cursory look and impressive to see One immediate thing that stands out, can you move your build-docker script to And thank you for jumping on this so fast, you're helping us get out of the woods :) |
Added 👍 |
|
Really nice draft PR @rycerzes. Thanks for the contribution. I haven't gone team yet I'm just exploring at a high level. My first question is why do we need I understand that we need to do a one time conversion of the legacy envs in this repo and in the wild, but I'm not sure if we need to maintain that code within the cli. We could just keep the functionality within |
It's a temporary command, which I will remove before the PR is merged. I had previously planned to migrate the existing envs in a different PR but it would be wise to update the envs in this PR itself |
|
Okay played around with things a bit. Love the changes you have made to standard template. One thing I think we haven't made clear enough, and we should make clear once this PR is merged is this: The new approach is for this repository to be the specification and CLI repo with some canonical examples of interesting examples (criteria for a good example environment is to demonstrate new specification or openenv core functionality). Going forward, all other environments, will be assumed to have been started with All CLI commands will be assumed to be running from the root of an So for your newly added commands can you remove the
etc. Additional feedback:
I'll add inline comments now that are in pursuit of this direction. |
| Examples: | ||
| # Build echo_env with default settings | ||
| $ openenv build src/envs/echo_env |
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.
Change to $ openenv build
| # Build with custom tag | ||
| $ openenv build src/envs/echo_env -t my-custom-tag | ||
| # Build and push to registry |
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.
dig this, but move to push. in other words: openenv push --registry myregistry.io/<path1>/<path2> etc.
| $ openenv build src/envs/echo_env | ||
| # Build with custom tag | ||
| $ openenv build src/envs/echo_env -t my-custom-tag |
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.
love it, for completion's sake: openenv build -t my-custom-tag
| Examples: | ||
| # Start echo_env on default port | ||
| openenv serve echo_env |
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.
openenv serve echo_env -> openenv serve and so on
|
|
||
|
|
||
| def validate( | ||
| env_name: str = typer.Argument( |
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.
kill
| Examples: | ||
| # Validate echo_env | ||
| openenv validate echo_env |
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.
openenv validate echo_env -> openenv validate
| 5. Required dependencies are present | ||
| Args: | ||
| env_path: Path to the environment directory |
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.
kill
|
|
||
| ```bash | ||
| # Install environment in editable mode | ||
| pip install -e ./my_env |
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.
pip install -e ./my_env -> pip install -e .
| pip install -e ./my_env | ||
|
|
||
| # Or using uv (faster) | ||
| uv pip install -e ./my_env |
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.
uv pip install -e ./my_env -> uv pip install -e .
| uv pip install -e ./my_env | ||
|
|
||
| # Run server locally without Docker | ||
| uv run --project ./my_env server --host 0.0.0.0 --port 8000 |
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.
uv run --project ./my_env server --host 0.0.0.0 --port 8000 -> uv run --project . server --host 0.0.0.0 --port 8000
|
One last thing, though you are killing off @rycerzes since you're closest to the new standard, can you write up Things to cover that come to mind:
|
|
@zkwentz Thanks for the detailed review! I understand the new CLI-first approach and will make all the requested changes Questions:
I'll have these changes ready in some time :) |
I see what you're saying, it's a bit wonky because we do a custom build for Hugging Face... it will be like that for some time as well. It's making sense to me now why you included a registry push on build instead of push itself. Would it be weird to have three optional arguments:
Resulting in the following options:
No preferences :) Thank you! |
This PR is for the issue Standardize Environment Directory Structure #145 . Major changes to dependency management, Docker build automation, and CLI usability for OpenEnv environments. The changes standardize the use of
pyproject.tomlfor dependencies, add scripts for generating Docker-compatible requirements, and enhance the CLI with new commands and validation utilities. The Docker base image is also updated to useuvfor faster dependency handling.Dependency Management and Build Automation
pyproject.tomlas the primary dependency specification for environments, clarified usage inREADME.md, and updated the root and corepyproject.tomlfiles to reflect modern Python and dependency requirements. [1] [2] [3]scripts/build_docker.pyto automate Docker image building, including requirements generation and optional image pushing to registries.Docker Image Improvements
uvfor dependency installation, improved caching, and modernized the build process for faster and more reliable builds.CLI Enhancements
convert,serve,buildandvalidate, in addition toinitandpush. [1] [2]convertcommand is a temporary script which will help generate the boilerplate code required for making the existing envs compatible with the new structureservecommand is for multi-mode deployment i.e., expose a pure python server runtime with dependency management without dockervalidate_env_structureto check environment directory structure and provide helpful warnings or errors, supporting the newvalidatecommand.TODOs:
convertcommand once all envs are migratedrequirements.txtfrom the existing envs