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

Re-implements network check with vendored psutil #1357

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

luiscape
Copy link
Member

Adds the same logic from this PR but with vendored psutil so we don't have to install an additional package.

@ekzhang
Copy link
Member

ekzhang commented Feb 21, 2024

Can you include the copyright notice?

https://github.com/giampaolo/psutil?tab=BSD-3-Clause-1-ov-file

vendor/psutil.py Outdated Show resolved Hide resolved
@mwaskom
Copy link
Contributor

mwaskom commented Feb 21, 2024

Also can we prefix the location with an underscore (e.g. _vendor so that it doesn't get confused for public API?

@luiscape luiscape force-pushed the luis/re-implements-network-check branch from 8df1f32 to 02a3693 Compare February 26, 2024 02:38
@mwaskom
Copy link
Contributor

mwaskom commented Feb 26, 2024

What is our approach to tests going to be with vendored code? Are we just going to assume that our existing unit tests cover all the behaviors that we need?

@mwaskom
Copy link
Contributor

mwaskom commented Feb 26, 2024

It also feels like we want some way of tracking what version the vendored code is sourced from. How will we know when we need to update it, e.g. if there is an important bug in the source repository?

@ekzhang
Copy link
Member

ekzhang commented Feb 26, 2024

While these are reasonable comments, I don't think they are relevant to this PR, as the use of psutil here is just a particular function that just reads and parses /proc for diagnostic reasons. We can revisit it if we need more functionality.

Other dependencies will need version tracking and so on, but this one is very easy to strip out.

@mwaskom
Copy link
Contributor

mwaskom commented Feb 26, 2024

Now is probably the only time we will know what version this corresponds to though :)

@ekzhang
Copy link
Member

ekzhang commented Feb 26, 2024

It shouldn't matter what version it corresponds to — barring any changes to the POSIX API, it should not need updates.

@luiscape
Copy link
Member Author

Are we just going to assume that our existing unit tests cover all the behaviors that we need?

This should be the case, yes. The tests I added test our interface and assumes the vendored code works as it should. That's is acceptable imo.

It also feels like we want some way of tracking what version the vendored code is sourced from.

I'll add a note to comments if only to point to the right license / code version. I agree with Eric that anything more is overkill.

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.

None yet

3 participants