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

use explainaboard.serialization.legacy in SDK 0.11.2 #334

Closed
wants to merge 1 commit into from

Conversation

qjiang002
Copy link
Collaborator

After updating SDK to version 0.11.2, the web cannot run locally due to import error

[0]   File "/Users/jiangqi/Desktop/Capstone/explainaboard_web/backend/src/gen/explainaboard_web/impl/db_utils/system_db_utils.py", line 17, in <module>
[0]     from explainaboard.utils.serialization import general_to_dict
[0] ModuleNotFoundError: No module named 'explainaboard.utils.serialization'
[0] npm run start-backend exited with code 1

In the new SDK, the general_to_dict function is in explainaboard.serialization.legacy.
Fix import error by using from explainaboard.serialization.legacy import general_to_dict

@lyuyangh
Copy link
Member

Thanks! Not sure if @OscarWang114 has started this, but I think we can maybe create a PR that bumps the SDK version and fixes all the breaking changes like this one?

@pfliu-nlp
Copy link
Collaborator

Also, here are:

Copy link
Collaborator

@pfliu-nlp pfliu-nlp left a comment

Choose a reason for hiding this comment

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

it looks good to me.

@qjiang002
Copy link
Collaborator Author

Thanks for the checklist. I find another error when submitting the system in this issue #336. This error doesn't relate to the import error in this PR. I'll go ahead to merge this one first.

@lyuyangh
Copy link
Member

@qjiang002 Just to clarify, are we running on 0.11.2 already? I think this repo is still using 0.11.1. If we merge this now, it would break our staging and production environment.

@qjiang002
Copy link
Collaborator Author

Thanks @lyuyangh for the reminder. The current requirements still uses 0.11.1. I will not merge this PR now and will put this change in another PR for fixing the bumping version issues together.

@OscarWang114
Copy link
Collaborator

Thanks @qjiang002 and @lyuyangh ! I haven't started, and yes feel free to submit another PR

@qjiang002 qjiang002 mentioned this pull request Sep 16, 2022
6 tasks
Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

In the future we should convert to the new serialization code, but I think this works for now.

@qjiang002 qjiang002 added the invalid This doesn't seem right label Sep 20, 2022
@neubig
Copy link
Contributor

neubig commented Sep 23, 2022

@qjiang002 : does the "invalid" tag mean that this will never be merged? If so I think you can close the PR.

@qjiang002 qjiang002 closed this Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants