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

Update file size to 500 kilobytes #5596

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Update file size to 500 kilobytes #5596

merged 1 commit into from
Sep 12, 2022

Conversation

stulle123
Copy link
Contributor

Hi there,

This PR is a result from a discussion in #5580 where @jeffwidman asked to split up the original PR into two.

The purpose of this PR is to increase the file size of requirements.txt files to 500 kilobytes.

Especially in large projects, pip-tools-compiled requirement.txt files can get pretty big.

As @dws mentioned here the ideal solution would be to have the file size as a configurable parameter.

If this change requires any unit tests please point me to existing examples.

Thanks!

@jeffwidman
Copy link
Member

jeffwidman commented Aug 29, 2022

Thanks for splitting that out!

Why the bump all the way to 500KB? Just how big is your requirements.txt file??? 😄
Keep in mind the reason for this is skip ingesting large docs files, so by bumping to 500K in the pathological case of a bunch of large files it can really go off the rails. I'd be tempted to start at a number that covers the case you're facing today with a little bit of buffer, and then from there we can later bump again if needed rather than preemptively jumping all the way to 500...

@deivid-rodriguez
Copy link
Contributor

After a second thought, I think this size limit change is acceptable without tests. It could be consider configuration after all, even if currently hardcoded. I think testing this kind of thing might be a bit overkill. What do you think?

In any case, I think the right way to go about this in the future is to make the manifest name configurable, and remove parsing all files of a certain size and guessing which of them are actually manifest files. Just parse the well known manifest files and let users explicitly configure more unusual situations. So in my opinion this file size should eventually go away anyways.

@jeffwidman
Copy link
Member

I'm +1 to no tests for now on this. I'll file an issue later today about non-standard manifest file names should require custom config and we can discuss more there once I write it up.

Also, after thinking about it more, I'm okay with going straight to 500... for most repos it'll have no effect, and for a few pathological ones it'll cause them to run out of memory but it'd take a lot of files for an extra 200KB to cause a problem unless we're making a lot of copies in memory of the file content... in which case the code could probably be cleaned up a little.

@jeffwidman jeffwidman self-assigned this Aug 31, 2022
@stulle123
Copy link
Contributor Author

Great! Anything I can still do here? :-)

@jeffwidman jeffwidman merged commit 8767b64 into dependabot:main Sep 12, 2022
@pavera pavera mentioned this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants