-
Notifications
You must be signed in to change notification settings - Fork 178
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
Improving Transform and Rerank Module #396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good! all important changes. Have left some comments. most minor, some might require little bit of work. Thanks!
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
45704de
to
775da63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed some of this PR - more to come tomorrow
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
if dataset_name is None: | ||
return None, None | ||
|
||
time.sleep(10) # To avoid rate limiting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't love this - it makes a very specific hardcoded assumption about the rate limit. What if we instead use an on-premise LLM like Llama2 with no rate limit, or we use a version of GPT with a very high rate limit? Then this will probably no longer be required
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a high-level, these changes look good. I've requested a bunch of small (but important) changes, mostly on minor implementation details, documentation, or code quality things
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
prompt2model/dataset_retriever/description_dataset_retriever.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Vijay Viswanathan <vijayv@andrew.cmu.edu>
Co-authored-by: Vijay Viswanathan <vijayv@andrew.cmu.edu>
Co-authored-by: Vijay Viswanathan <vijayv@andrew.cmu.edu>
Co-authored-by: Vijay Viswanathan <vijayv@andrew.cmu.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too! Great job!
Description
Major Changes:
Minor Changes/Fixes:
Testing
Tested with dummy example + pytest
References