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

add folder http endpoint #1108

Merged
merged 1 commit into from
Aug 12, 2024
Merged

add folder http endpoint #1108

merged 1 commit into from
Aug 12, 2024

Conversation

jlewitt1
Copy link
Collaborator

@jlewitt1 jlewitt1 commented Aug 5, 2024

No description provided.

Copy link

sentry-io bot commented Aug 5, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: runhouse/servers/http/http_utils.py

Function Unhandled Issue
handle_response TypeError: to() received an invalid combination of arguments - got (OnDemandCluster, name=str, env=Env), but... ...
Event Count: 35
handle_response ModuleNotFoundError: No module named 'TorchExampleModelClass' _main...
Event Count: 21
handle_response StopIteration main in
Event Count: 12
handle_response ImportError: cannot import name 'ValidationInfo' from 'pydantic' (/opt/conda/lib/python3.10/site-packages/pyda... ...
Event Count: 11

Did you find this useful? React with a 👍 or 👎

Copy link
Collaborator Author

jlewitt1 commented Aug 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jlewitt1 and the rest of your teammates on Graphite Graphite

)

for filename, file_obj in contents.items():
binary_mode = "b" in mode
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dongreenberg marking this now to discuss later, the server serialization / deserialization business for file contents getting and putting feels a little hairy

@jlewitt1 jlewitt1 force-pushed the folder-http-endpoint branch 3 times, most recently from 22cb540 to 4cf2141 Compare August 6, 2024 16:37
@jlewitt1 jlewitt1 force-pushed the folder-http-endpoint branch 2 times, most recently from 314dcb3 to 9794d5a Compare August 6, 2024 17:30
@jlewitt1 jlewitt1 marked this pull request as ready for review August 6, 2024 21:44
@jlewitt1 jlewitt1 force-pushed the folder-http-endpoint branch 2 times, most recently from 4ad4f88 to ca83d03 Compare August 8, 2024 22:19
@@ -612,6 +624,92 @@ async def rename_object(request: Request, params: RenameObjectParams):
e, traceback.format_exc(), from_http_server=True
)

@staticmethod
@app.post("/folder/method/ls")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just /folder/ls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this conflicts with the @app.post("/{key}/{method_name}") method, but welcome to other ideas for the naming convention we use for these endpoints

)


def folder_rm(path: Path, contents: List[str], recursive: bool):
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit confused on contents and path both being options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this supports being able to specify specific folder contents to remove instead of just the entire folder itself, you think that's overkill?

@jlewitt1 jlewitt1 force-pushed the folder-http-endpoint branch 2 times, most recently from bbd72e7 to e5e16e7 Compare August 10, 2024 20:42
Copy link
Collaborator Author

jlewitt1 commented Aug 12, 2024

Merge activity

  • Aug 12, 4:15 PM EDT: @jlewitt1 started a stack merge that includes this pull request via Graphite.
  • Aug 12, 4:17 PM EDT: Graphite rebased this pull request as part of a merge.
  • Aug 12, 4:18 PM EDT: @jlewitt1 merged this pull request with Graphite.

@jlewitt1 jlewitt1 merged commit 0232d57 into main Aug 12, 2024
12 of 13 checks passed
@jlewitt1 jlewitt1 deleted the folder-http-endpoint branch August 12, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants