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

[ENH] Update onnx_mini_lm_l6_v2.py to support custom timeout #2911

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmerkle
Copy link

@bmerkle bmerkle commented Oct 8, 2024

fix for #2910

specify timeout, as start of download might take some time because of corporate proxies, firewalls, content scanning etc.

Description of changes

Summarize the changes made by this PR.
-specifed timeout in httpx call

  • start of download might take some time because of corporate proxies, firewalls, content scanning etc
  • it failed with timeout

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

fix for chroma-core#2910

specify timeout, as start of download might take some time because of corporate proxies, firewalls, content scanning etc.
Copy link

github-actions bot commented Oct 8, 2024

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)

@@ -97,7 +97,7 @@ def _download(self, url: str, fname: str, chunk_size: int = 1024) -> None:
which makes mypy unhappy. If some smart folk knows how to fix this in an
elegant way, please do so.
"""
with httpx.stream("GET", url) as resp:
with httpx.stream("GET", url, timeout=None) as resp:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we'd want to unilaterally set timeout to None. The default is 5 seconds - https://www.python-httpx.org/advanced/timeouts/.

Could we instead make this a parameter at EF construction time that one can set?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @HammadB
fully agree that None does not make sense.

I think the best would be to offer some configuration value or environment variable which can be set matching the timings of the infrastructure (as this might vary between users). The best would be if httpx offers this out of the box.

In our case there was some scanning of images which happens during the download so the initial download takes some time (to be secure) and later the stuff is cached and immediately available.

@HammadB HammadB changed the title Update onnx_mini_lm_l6_v2.py [ENH] Update onnx_mini_lm_l6_v2.py to support custom timeout Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants