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

Vendor pympler.asizeof #7193

Merged
merged 11 commits into from Aug 23, 2023
Merged

Conversation

rudyardrichter
Copy link
Contributor

To resolve #7131. This adds pympler's asizeof module to vendor and removes the pympler dependency.

For testing plan: I expect this is covered by existing tests since this is an in-place replacement which already has test coverage.

The changes to the original pympler code are pretty minimal—let me know if further cleanup would be appreciated! I removed some things that were clearly unnecessary to support only asizeof but didn't look too closely otherwise.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@vdonato
Copy link
Collaborator

vdonato commented Aug 17, 2023

Thanks for the contribution @rudyardrichter! At a quick glance, this LGTM, but I'll want to double check the diff between the vendored asizeof and original code before giving this PR an official approval. I'll also most likely wait until some time next week before approving and hitting merge on this PR so that the change goes out in 1.27 rather than the upcoming release (the release cutoff for 1.26 is tomorrow). While this change seems low risk to me, I'd prefer to not immediately include it in a release simply because we don't have any other instances of us vendoring Python code like this.

One other small ask: could you write a quick summary of what was removed from the original asizeof.py in the docstring in the vendored module?

@rudyardrichter
Copy link
Contributor Author

Thanks @vdonato! I added a snippet to the docstring.


The original module docstring that appears in pympler follows; note that some of
it no longer pertains here, but it's preserved to document implementation
details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a must, just a nit pick.

@rudyardrichter possibly should change this to be code comments instead of being within triple quotes as using this format clobbers the original python module docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's probably fine to clobber it since this one is the documentation as relevant to streamlit, but I could see going either way with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, happy to defer here 🙂

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks for the help with this one @rudyardrichter and @SimonBiggs!

We'll have this change out with v1.27.0

@vdonato vdonato merged commit 2b2886e into streamlit:develop Aug 23, 2023
50 checks passed
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
* Vendor pympler.asizeof module

* Remove pympler from min constraints

* Explain differences from original asizeof module

* include

* Ignore mypy errors within asizeof.py

* Fix asizeof usage in cache_resource_api

* Have CodeQL ignore vendored Python code

---------

Co-authored-by: SimonBiggs <simon@anthropic.com>
Co-authored-by: Vincent Donato <vincent@streamlit.io>
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
* Vendor pympler.asizeof module

* Remove pympler from min constraints

* Explain differences from original asizeof module

* include

* Ignore mypy errors within asizeof.py

* Fix asizeof usage in cache_resource_api

* Have CodeQL ignore vendored Python code

---------

Co-authored-by: SimonBiggs <simon@anthropic.com>
Co-authored-by: Vincent Donato <vincent@streamlit.io>
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
* Vendor pympler.asizeof module

* Remove pympler from min constraints

* Explain differences from original asizeof module

* include

* Ignore mypy errors within asizeof.py

* Fix asizeof usage in cache_resource_api

* Have CodeQL ignore vendored Python code

---------

Co-authored-by: SimonBiggs <simon@anthropic.com>
Co-authored-by: Vincent Donato <vincent@streamlit.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pympler dependency is no longer maintained
4 participants