-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add get_bytes_range()
function
#196
Conversation
|
||
# If we're using the /a/b/foo.zip!c/d/file.txt syntax, handle it here. | ||
exclamation_index = url_or_filename.find("!") | ||
if extract_archive and exclamation_index >= 0: |
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.
if extract_archive
is true and there is no !
, we should probably return some sort of error/exception
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.
It's possible it could be part of the filename though, no? I think this is how cached_path()
behaves so I wanted to match the behavior.
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.
What if an archive file has an !
in its name? It's an edge case but would cause things to break. Having a separate optional parameter for the relative path of the file within the archive would be a way to get around the problem
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.
Having a separate optional parameter for the relative path of the file within the archive would be a way to get around the problem
I agree but I don't want to deviate from the API of cached_path()
. At least for now.
We use a function like this a lot in the LLM repo so I thought we should add it here.