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.

Sign up for GitHub

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

Jump to bottom

[BUG] fix bug when receive embedding from vertex api #2000

Closed
wants to merge 7 commits into from
Closed

[BUG] fix bug when receive embedding from vertex api #2000

wants to merge 7 commits into from

Conversation

pig7788
Copy link

@pig7788 pig7788 commented Apr 11, 2024 •
edited
Loading

  1. fix the way to parse response using list-parsing instead of dict-parsing.
    see issue here.

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • fix the way to parse response from vertex api using list-parsing instead of dict-parsing

[fix] the bug when receive embedding from vertex api
3dd01d2 
1. fix the way to parse response using list-parsing instead of dict-parsing.
@github-actions GitHub Actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@pig7788 pig7788 changed the title [fix] the bug when receive embedding from vertex api [fix] bug when receive embedding from vertex api Apr 11, 2024
@pig7788 pig7788 changed the title [fix] bug when receive embedding from vertex api [BUG] bug when receive embedding from vertex api Apr 12, 2024
@pig7788 pig7788 changed the title [BUG] bug when receive embedding from vertex api [BUG] fix bug when receive embedding from vertex api Apr 12, 2024
tazarov
tazarov requested changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@tazarov tazarov left a comment

Choose a reason for hiding this comment

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

@pig7788, can you run a pre-commit run to clean up some weird formatting? There are a lot of changes, and it is difficult to make sense of what you made.

Note: you need to install dev deps pip install -r requirements_dev.txt

@pig7788
Copy link
Author

pig7788 commented Apr 15, 2024

@tazarov These weird changes were fixed when I rebase the previous commit. Formatting tool leads to these changes. Thank you for your reminder.

@tazarov
Copy link
Contributor

tazarov commented Apr 16, 2024

@pig7788, this looks better. Thank you. Do you think we can also add maybe some sort of error if we don't find the response we expect? I think it would make for a good developer experience instead of returning an empty embeddings list.

@pig7788
Copy link
Author

pig7788 commented Apr 18, 2024

@pig7788, this looks better. Thank you. Do you think we can also add maybe some sort of error if we don't find the response we expect? I think it would make for a good developer experience instead of returning an empty embeddings list.

@tazarov It is a good idea. I will add this in my code. Thanks!

[fix] the bug when receive embedding from vertex api
f0bea38 
1. rewrite the procedure of recieving response and make it more readable.
2. add some checks to confirm which conditions that developers encounter.
@pig7788 pig7788 requested a review from tazarov April 22, 2024 05:40
Albert Li added 2 commits April 23, 2024 15:52
Update: chroma-core#2000
91a4446 
Summary of changes:
1. add AuthorizationError and msg for hint
2. add response as a part of error msg for reminder.
Update: chroma-core#2000
93b657e 
Summary of changes:
1. cancel AuthorizationError and msg for hint (because of reducing the complexity of hints.)
beggers
beggers reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@beggers beggers left a comment

Choose a reason for hiding this comment

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

A few nits and then we're good to check this in!

chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
tazarov
tazarov requested changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@tazarov tazarov left a comment

Choose a reason for hiding this comment

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

@pig7788, on top of Ben's nits. Can you also write some tests to verify the EF's behavior? E.g.:

  • Normal test that succeeds
  • Missing API key
  • Any other important error condition that might be useful

Put your tests under chromadb/test/ef. See here for an example - https://github.com/chroma-core/chroma/pull/1271/files#diff-34ec2c15f530afc997a54ec898b37eea1c56dd0c70c7474d2a5ae10149bf810d

Update: chroma-core#2000
cc18c22 
    Summary of changes:
    1. add unit test
    2. add checks for parameters.
    3. change the response checks and wordings.
@vercel Vercel
Copy link

vercel bot commented May 10, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
chroma ❌ Failed (Inspect) May 13, 2024 3:26am

Update: chroma-core#2000
be5329d 
        Summary of changes:
        1. add test case for missing api key.
        2. add check for api key.
        3. change the way when calling auth error.
@pig7788
Copy link
Author

pig7788 commented May 10, 2024

@tazarov I add test cases that you advised and I could think of. Thank you for your suggestions. Thank @beggers too.

tazarov
tazarov requested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@tazarov tazarov left a comment

Choose a reason for hiding this comment

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

@pig7788, a few more changes, but we're nearly there. Thank you for the perseverance 🥇

chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
chromadb/test/ef/test_vertex_ef.py Show resolved Hide resolved
chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
Update: chroma-core#2000
edf580d 
Summary of changes:
1. revise test case of testing api key.
2. remove default value from project_id.
3. rewrite condition and exception content to check response.
@pig7788
Copy link
Author

pig7788 commented May 15, 2024

@tazarov I commit changes that you suggested, please confirm it, thx.

@tazarov tazarov mentioned this pull request May 15, 2024
Merged
4 tasks
@jeffchuber
Copy link
Contributor

Our underlying impl has changed and so this PR is not landable as is.

That being said - we'd still like to add this functionality and that is now tracked in this issue.

@jeffchuber jeffchuber closed this Sep 15, 2024
@jeffchuber jeffchuber mentioned this pull request Sep 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beggers beggers beggers left review comments

@tazarov tazarov tazarov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pig7788 @tazarov @jeffchuber @beggers