Input and display children #203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
By the way, this is stacked on top of #202, just because I dont want to deal with potential merge conflict |
codekansas
requested changes
Jul 30, 2024
| @@ -109,7 +113,7 @@ async def upload_mjcf( | |||
| ) -> Artifact: | |||
| artifact = Artifact.create( | |||
| user_id=user_id, | |||
| artifact_type="urdf", | |||
| artifact_type="mjcf", | |||
store/app/routers/listings.py
Outdated
| @@ -35,6 +35,21 @@ async def list_listings( | |||
| return await crud.get_listings(page, search_query=search_query) | |||
|
|
|||
|
|
|||
| @listings_router.get("/batch") | |||
| async def batch( | |||
There was a problem hiding this comment.
nit: for function names, prefer doing something like <verb>_<something>, e.g., get_batch
store/app/routers/urdf.py
Outdated
Comment on lines
25
to
40
| @urdf_router.post("/upload") | ||
| async def upload_urdf( | ||
| user: Annotated[User, Depends(get_session_user_with_write_permission)], | ||
| crud: Annotated[Crud, Depends(Crud.get)], | ||
| file: UploadFile, | ||
| ) -> UserInfoResponse: | ||
| if file.content_type != "application/gzip": | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Only tar.gz files containing a URDF are supported", | ||
| ) | ||
| artifact = await crud.upload_urdf( | ||
| file=file.file, | ||
| user_id=user.id, | ||
| ) | ||
| return UserInfoResponse(urdf_id=artifact.id) |
There was a problem hiding this comment.
question - it seems like this lets us upload multiple urdfs per listing, but then the listing endpoint only returns a single urdf?
There was a problem hiding this comment.
now only returns the latest urdf
store/app/crud/listings.py
Outdated
Comment on lines
64
to
70
| if len(urdfs) == 0: | ||
| return None | ||
| if len(urdfs) > 1: | ||
| raise InternalError( | ||
| f"""More than one URDF found for listing {listing_id}. | ||
| This is due to incorrect data in the database, likely caused by a bug.""" | ||
| ) |
There was a problem hiding this comment.
in the upload flow, it seems like there is nothing stopping us from uploading multiple urdfs per listing - we should reflect this in the crud side of things
… listing page (#211) * Add route to fetch artifacts in bulk and return only images * In Listings, fetch images from artifact_ids This fixes captions being just "caption" (which was a placeholder waiting for this commit) and also fixes trying to render URDF tar gunzips. * Add 'name' field to Artifacts, which stores filename This is so people have a good identifier for URDFs, etc. * Take in a listing for upload_urdf - Check whether user actually owns that listing - Update listing artifact_ids * rename batch to get_batch * Change route that gets unique urdf to route that gets latest
codekansas
pushed a commit
that referenced
this pull request
Jul 30, 2024
* Add functionality to set children * Display children * Fetch images from artifact ids and render their captions correctly in listing page (#211) * Add route to fetch artifacts in bulk and return only images * In Listings, fetch images from artifact_ids This fixes captions being just "caption" (which was a placeholder waiting for this commit) and also fixes trying to render URDF tar gunzips. * Add 'name' field to Artifacts, which stores filename This is so people have a good identifier for URDFs, etc. * Take in a listing for upload_urdf - Check whether user actually owns that listing - Update listing artifact_ids * rename batch to get_batch * Change route that gets unique urdf to route that gets latest
codekansas
added a commit
that referenced
this pull request
Jul 31, 2024
* Add upload URDF functionality We now upload URDFs as .tar.gz files. The URDF loader has not been implemented yet. The plan is to untar the URDF on the client. See something like https://github.com/InvokIT/js-untar * Functionality to fetch urdf * fix mjcf upload type * upload urdf to s3 with get_urdf_name as filename * merge image and urdf base urls into artifact base url * fix urdf content-type: xml => gunzip * Input and display children (#203) * Add functionality to set children * Display children * Fetch images from artifact ids and render their captions correctly in listing page (#211) * Add route to fetch artifacts in bulk and return only images * In Listings, fetch images from artifact_ids This fixes captions being just "caption" (which was a placeholder waiting for this commit) and also fixes trying to render URDF tar gunzips. * Add 'name' field to Artifacts, which stores filename This is so people have a good identifier for URDFs, etc. * Take in a listing for upload_urdf - Check whether user actually owns that listing - Update listing artifact_ids * rename batch to get_batch * Change route that gets unique urdf to route that gets latest * quick fix * periodic commit * no more errors * no more errors * some listing details * use react links instead of href * fix lint * ignore gen files * instructions in contributing.md file * almost there * fix tests * add api * fix another test --------- Co-authored-by: Benjamin Bolte <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Write some api routes such and do some frontend work so that
you may now upload children to listings
each child of a listing is displayed
