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

Swap with drop-in API replacement working on Windows as well #2

Closed
wants to merge 4 commits into from

Conversation

vnlitvinov
Copy link
Contributor

@vnlitvinov vnlitvinov commented Nov 24, 2020

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Replace blessings with blessed which is an API-compatible fork that works on Windows as well as on other platforms.
Upstream gpustat has already moved to it but has not made a release yet.

@vnlitvinov vnlitvinov requested a review from hadim as a code owner November 24, 2020 11:45
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • noarch: python recipes are recommended to have a lower bound on the python version. This recommendation will become a requirement in the future.

@vnlitvinov vnlitvinov marked this pull request as draft November 24, 2020 11:45
@vnlitvinov
Copy link
Contributor Author

I was too hasty on pushing "create" button, need to prepare some source patches. Converting to draft...

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@vnlitvinov
Copy link
Contributor Author

@conda-forge-admin please rerender

@vnlitvinov vnlitvinov marked this pull request as ready for review November 24, 2020 19:09
@vnlitvinov
Copy link
Contributor Author

@conda-forge/gpustat this is ready for review

@hadim
Copy link
Member

hadim commented Nov 24, 2020

There are some important modifications made here that does not really reflect the current version of gpustat (0.6.0). I am not comfortable merging into the 0.6.0 version of the conda package that has been released more than one year ago now.

Can you ask upstream to release a new version and we will just build a new package here once this is done? In the meantime, you should rely on a pip install from master.

pinging @conda-forge/help-python in case they have an opinion different than mine here.

@vnlitvinov
Copy link
Contributor Author

I can technically revert most of Windows-specific patches, as they were relevant for running the thing on Windows (and won't trigger in the CI).
However, as my main point was enabling this on Windows, I decided to add them 😃

I sure will contact the upstream.

@vnlitvinov
Copy link
Contributor Author

@hadim
Also do note that this PR adds running gpustat tests (which helped uncovering all the things), which adds to the amount of changed lines.

@hadim
Copy link
Member

hadim commented Nov 24, 2020

Agree with the tests and thanks for the work!

But my point is we should not change the deps of the version of the package neither add a patch of any sort nor it could break other person pipeline if they rely on this specific version.

I am fine merging the added tests, bumping the build number but without the patches and the switch to blessed. For this, I would wait for the upstream release.

@vnlitvinov
Copy link
Contributor Author

@hadim extracted re-render and tests into #3

@vnlitvinov vnlitvinov marked this pull request as draft November 24, 2020 19:58
@vnlitvinov
Copy link
Contributor Author

Rebased off new master, converted into draft for now - waiting for upstream response.

@hadim
Copy link
Member

hadim commented Nov 24, 2020

Thanks, @vnlitvinov for your time and being comprehensive!

@h-vetinari
Copy link
Member

There's been a tag of 1.0.0b1 and wookayin/gpustat#78 shortly after. It's been more than half a year since then - I've asked upstream about the plan.

BTW, saw this PR too late before opening #4, sorry.

@mattip
Copy link
Contributor

mattip commented Oct 6, 2022

gpustat 1.0.0 supports windows

@mattip mattip mentioned this pull request Oct 11, 2022
5 tasks
@hadim hadim closed this Oct 11, 2022
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.

5 participants