-
Notifications
You must be signed in to change notification settings - Fork 80
[UTIL] deployment script for single env on any namespace #102
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
Conversation
|
@burtenshaw - Can we try to reconcile .github/workflows/deploy_to_hf.yaml + existing script with this new script? I could be wrong but is it feasible to use this script and call it from deploy_to_hf.yaml and remove the previous bash script - |
|
@burtenshaw thank you for this PR, +1 to @pankit-eng comment. Additional deployment logic improvements should be addressed in a separate RFC focused on creating and maintaining OpenEnv environments (e.g., openenv init, openenv push). While the example environments here are valuable, the real democratization happens through the hub. Here's what we need to build next: Goal: Enable developers to push OpenEnv environments to the HuggingFace OpenEnv Hub
I'll provide full details in the upcoming RFC, but wanted to share this preview to get the conversation started. |
Yeah, that's totally possible. I deliberately separated them for ease. I'll upgrade this PR to keep the functionality of |
The rfc looks excellent, and this project style definition (openenv init, openenv push) will be very familiar to devs. In practical terms, I expect we can just reimplement much of this bash logic as a real cli. |
6d6463e to
ae7c547
Compare
|
I'm experimenting with having Claude Code do code reviews. What I normally do is run it first, and then review myself. For this particular PR, I'm not the best person to review it but I'm just copypasting what Claude is saying in case this helps. Comprehensive Review: PR #102 - HF Deployment Script Summary PR #102 adds scripts/deploy_to_hf.sh, a new end-to-end deployment script for pushing OpenEnv environments to Hugging Face Spaces. This enables community contributors to deploy their Status: Good contribution, but needs architectural decision about relationship to existing deployment infrastructure. The Core Question (@pankit-eng) "Can we reconcile .github/workflows/deploy_to_hf.yaml + existing script with this new script?" Short answer: Yes, and you should. Current State Analysis Existing Infrastructure:
New Script (PR #102): scripts/deploy_to_hf.sh (527 lines)
Key Differences
The Good ✅
Before: Only Meta team could deploy to huggingface.co/spaces/openenv/
Old way: Not possible for community contributors(requires GitHub Actions + secrets)New way: Simple one-linerbash scripts/deploy_to_hf.sh --env my_custom_env
The hf CLI is the official recommended way to interact with HF Spaces now (vs git clones).
The Issues
You now have TWO ways to deploy to HF:
This creates maintenance burden. When you add a new environment (like textarena_env or wildfire_env), you need to update:
Lines 176-262 have hardcoded Dockerfile templates for specific envs: This means:
Same issue in README generation (lines 296-473).
The old prepare_hf_deployment.sh copies the environment's existing Dockerfile (src/envs/$ENV_NAME/server/Dockerfile), which is what's actually tested and maintained. The new script generates Dockerfiles from templates, which could drift from reality.
The hardcoded list is already out of date:
Architectural Recommendations Option A: Merge and Refactor (Recommended) Create a unified approach:
Benefits:
Option B: Keep Both, Document Use Cases Keep both scripts with clear separation:
Benefits:
Drawbacks:
Specific Code Issues
if ! hf auth whoami | grep -qw "$HF_NAMESPACE"; then Problem: hf auth whoami returns the authenticated username, NOT a list of orgs/namespaces they have access to. This check will ALWAYS fail if HF_NAMESPACE is an organization name Fix: Remove this check or query HF API properly to check org membership.
"set if with 'hf auth login'"
if [ -d "$CURRENT_STAGING_DIR_ABS" ]; then This only removes the specific env's staging dir, not the parent hf-staging/ directory. After deploying 10 environments, you'll have hf-staging/ with 10 subdirectories. Fix: Either clean up the parent or document it.
If someone creates src/envs/my_awesome_env/, the generated Dockerfile will be the generic template, which might not have the right dependencies. Better approach: Check if src/envs/$ENV_NAME/server/Dockerfile exists, and if so, use it (like old script does). Recommendation Summary Immediate Action:
If Going with Option A (Merge): Request the contributor to:
If Going with Option B (Keep Both):
Draft Feedback Thanks for this contribution @burtenshaw! This is a valuable improvement that enables community contributors to deploy their environments to HF Spaces. @pankit-eng raises a good question about reconciling with existing infrastructure. Here's my analysis: Current SituationWe have three pieces:
Recommendation: Merge and RefactorI recommend we consolidate around this new script (it's better designed) but refactor it to work for both use cases: Changes Needed:
Next Steps:
This gives us:
|
|
This now makes the I've tested it by deploying a test env here with this run note:
|
|
@zkwentz - if you want to generate a new token i can update the secret |
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.
Much cleaner, thank you @burtenshaw!
This PR adapts
scripts/prepare_hf_deployment.shto take parameters env and namespace. This will allow community contributors to deploy their envs. Main changes from the original script are:hfcli tool.rmthe staging dirTo test this, you need to do:
out: