-
Notifications
You must be signed in to change notification settings - Fork 245
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
Emrgnt cmplxty patch 1 #253
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
❌ Changes requested.
- Reviewed the entire pull request up to bb4af63
- Looked at
48
lines of code in2
files - Took 48 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_BYPpzDT5iBV2Msas
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
CMD ["uvicorn", "r2r.examples.basic.app:app", "--host", "0.0.0.0", "--port", "8000", "--workers", "8"] |
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.
Consider making the number of workers configurable instead of hard-coding it to 8. This can be done by using an environment variable.
sess.execute(text("create schema if not exists vecs;")) | ||
except Exception as e: | ||
if "already exists" not in str(e): | ||
raise ValueError( |
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.
Consider raising a more specific exception type or a custom exception instead of ValueError. ValueError is typically used for different kinds of issues.
raise ValueError( | ||
f"Error {e} occurred while creating the 'vector' extension." | ||
) | ||
|
||
def _supports_hnsw(self): |
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.
The removal of the line that fetches the 'vector' extension version might cause an AttributeError in the '_supports_hnsw' method. Consider adding it back.
This reverts commit f9420c0.
* Update Dockerfile * Update client.py
This reverts commit f9420c0.
Summary:
This PR enhances the application's performance by enabling multi-worker processes in the Dockerfile and improves error handling in the initialization of the
Client
class inclient.py
.Key points:
/Dockerfile
to include--workers
flag in CMD command__init__
method ofClient
class in/r2r/vecs/client.py
Generated with ❤️ by ellipsis.dev