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

Fix store info display #3913

Merged
merged 29 commits into from
Dec 14, 2021
Merged

Fix store info display #3913

merged 29 commits into from
Dec 14, 2021

Conversation

jzwang43
Copy link
Contributor

@jzwang43 jzwang43 commented Dec 13, 2021

Reasons for making this change

Legacy bundles will not have bundle stores so we won't show the store info if API returns empty.

BundleLocationListSchema(many=True).dump doesn't seem to be working so I'm using json.dumps for now.

Related issues

fixes #3910

Screenshots

For an old file upload:
image

New bundle:
image

Checklist

  • I've added a screenshot of the changes, if this is a frontend change
  • I've added and/or updated tests, if this is a backend change
  • I've run the pre-commit.sh script
  • I've updated docs, if needed

@jzwang43 jzwang43 changed the title Fix store info Fix store info display Dec 13, 2021
@jzwang43 jzwang43 self-assigned this Dec 13, 2021
@epicfaace
Copy link
Member

Looks good except for the BundleLocationListSchema(many=True).dump(bundle_locations).data part. Why didn't that work? did you get a specific error? Ideally, we would use that instead of using json.dump.

@jzwang43
Copy link
Contributor Author

The schema is defined incorrectly I believe. The error is Unexpected Internal Error (Must have an id field). The administrators have been notified.

@epicfaace
Copy link
Member

Are you sure that's the error you get? BundleLocationListSchema doesn't have an id field defined (https://github.com/codalab/codalab-worksheets/blob/fix-store-info/codalab/rest/schemas.py#L247-L255) (only BundleStoreSchema does)

@jzwang43
Copy link
Contributor Author

Are you sure that's the error you get? BundleLocationListSchema doesn't have an id field defined (https://github.com/codalab/codalab-worksheets/blob/fix-store-info/codalab/rest/schemas.py#L247-L255) (only BundleStoreSchema does)

Yes I can confirm that's the error. If you look at the definition of the Schema class the BundleLocationListSchema extends, it requires an id field to be defined.

@epicfaace
Copy link
Member

epicfaace commented Dec 14, 2021

Are you sure that's the error you get? BundleLocationListSchema doesn't have an id field defined (https://github.com/codalab/codalab-worksheets/blob/fix-store-info/codalab/rest/schemas.py#L247-L255) (only BundleStoreSchema does)

Yes I can confirm that's the error. If you look at the definition of the Schema class the BundleLocationListSchema extends, it requires an id field to be defined.

Can you try reverting the BundleLocationListSchema(many=True).dump(bundle_locations).data -> json.dump change? I think the json change you did is causing the CI tests to fail. We should maybe get on a call to talk through this further to see what exactly the issue.

@jzwang43
Copy link
Contributor Author

jzwang43 commented Dec 14, 2021

Are you sure that's the error you get? BundleLocationListSchema doesn't have an id field defined (https://github.com/codalab/codalab-worksheets/blob/fix-store-info/codalab/rest/schemas.py#L247-L255) (only BundleStoreSchema does)

Yes I can confirm that's the error. If you look at the definition of the Schema class the BundleLocationListSchema extends, it requires an id field to be defined.

Can you try reverting the BundleLocationListSchema(many=True).dump(bundle_locations).data -> json.dump change? I think the json change you did is causing the CI tests to fail. We should maybe get on a call to talk through this further to see what exactly the issue.

If I revert the json.dumps change then the endpoint wouldn't work and this page will be broken.
The previous API I used was an old one and doesn't support multiple bundle locations.

@@ -3090,6 +3090,7 @@ def get_bundle_locations(self, bundle_uuid: str) -> List[dict]:
rows = connection.execute(
select(
[
cl_bundle_store.c.id,
Copy link
Member

Choose a reason for hiding this comment

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

let's revert this file (which I changed in an attempt to fix the json issue)

@mergify mergify bot merged commit 8ccf899 into master Dec 14, 2021
@mergify mergify bot deleted the fix-store-info branch December 14, 2021 23:25
@jzwang43 jzwang43 mentioned this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle store frontend error
2 participants