-
Notifications
You must be signed in to change notification settings - Fork 4
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
Initial Migration to Github Actions #34
Initial Migration to Github Actions #34
Conversation
Instead of splitting the frontend/backend, we should just migrate to Github Actions first and have the structure in place to split things post-migration (if necessary). @louisabraham I'm hoping that you can handle the twine uploading part once this PR gets merged. It'll require changing the last few lines of the github-actions.yml: pydivsufsort/.github/workflows/github-actions.yml Lines 71 to 80 in 56264bb
|
Regarding the release here's an example of a workflow for it : https://github.com/PyCQA/pylint/blob/main/.github/workflows/release.yml This will release the package when the release is created in github (tag created then release created by editing the tag description in interface). This can be copy pasted, only the value for: TWINE_REPOSITORY: pypi
TWINE_USERNAME: __token__
TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }} needs to be added in the project settings. |
Thanks @Pierre-Sassoulas. Frankly, @louisabraham I am not comfortable with handling this step as it will require upping the package version (even for |
You can test the release by creating an alpha version with a tag: |
My comfort level has only ever-so-slightly increased with this knowledge. 😄 @Pierre-Sassoulas Do you think this is something that you can help us with after this PR is merged? I am limited on time and, for this PR, I want to focus on the last major item that will allow us to have stable builds and at least put us in a better position to make progress:
|
I don't have enough time to do it myself but I can help you with it if you have questions. Also I have an example of such a migration: prospector-dev/prospector#427 Pylint also has a coverage job too if you want some inspiration: https://github.com/PyCQA/pylint/blob/main/.github/workflows/ci.yaml#L228 Regarding the minimum numpy version it depends on the deprecation of numpy and of the API you're using I don't know much about numpy nor what you're using. Determining the absolute minimum number might take some time. The lazy bet would be to support what users asks for and do it and test that it works when they do ? (Right now it seems it's numpy 1.19.5 for tensorflow 2.6.0) |
I went through all of the
|
@louisabraham As you can see, there isn't really a "true" minimum version of NumPy that would satisfy all operating systems. Can you please advise on what you'd want me to do? |
Let's do 1.19.5.
We can also put instructions for people who get errors so that they can
build from source.
…On Tue, Nov 9, 2021, 03:03 Sean M. Law ***@***.***> wrote:
@louisabraham <https://github.com/louisabraham> As you can see, there
isn't really a "true" minimum version of NumPy that would satisfy all
operating systems. Can you please advise on what you'd want me to do?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEQQFJM5T6LEK5VL3RSS43ULB6N5ANCNFSM5HCZMXIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Unfortunately, with
I'm trying |
@louisabraham I'm encountering the same cibuildwheel issue that you've run into before:
However, in my case (see Github Actions log), after building the I don't know if this is a distraction but, in my Github Actions workflow, I don't have |
Well I don't have a windows setup anymore so I'm not sure I can efficiently help. Isn't it working now? I really want to provide binaries for windows as it is one major feature of pydivsufsort compared to other project: provide a one line installation of divsufsort on all platforms. |
Yeah, similarly, I work on a Mac and only have access to Windows via what is available on Github Actions.
The wheels (built with My hypothesis is that maybe for Github Actions I need to pass the
In case it matters, I understand and I am not advocating for the removal of Windows support. I just don't have any experience here and am debugging through Github Actions. |
Would it be possible to create an env variable to give to the |
Sure but I think the issue is that we are not sure yet about a fix
…On Wed, Nov 10, 2021, 15:30 Pierre Sassoulas ***@***.***> wrote:
Would it be possible to create an env variable to give to the -A option
so it's defined properly for each run?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEQQFJWIFBZC2LXPLKGFS3ULJ6ZRANCNFSM5HCZMXIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
@Pierre-Sassoulas That's what I'm thinking as well (and this is what is being done for TravisCI). However, I'm struggling (due to my inexperience) to figure out how I can set the env variable |
I have posted the question with the |
I can confirm that setting |
Okay, I split the
|
@louisabraham I think this PR is ready for your review |
That's AWESOME !!! Thank you so much !!! There is still the question of M1 binaries but let's wait for a more stable support in CIBW. I'll have a look at pypi upload |
No, thank you for making this package available! I've enjoyed our collaboration
Agreed
If you look at this new github-actions.yml file (it's very similar to the Also, I didn't get a chance to add the code coverage so I hope that you can take a look. Right now, every PR or commit will trigger this Github Actions workflow so you might want to set up different workflows (i.e., for releases, for checking PRs, etc) |
Thanks, great job guys! As for numpy version, from what I understood from cython guys answers is that you just need to find the maximal of the minimums for all the platforms of interest. Judging by your log, it is 1.19 if you want to support py39 and up only, or 1.17 for py38+, or 1.14 for py37+ (it is surprising the build with the config I changed to 1.14 failed). Note also that this is the numpy version you only need for the build. It is not a formal requirement of the package because it just fixes the C interface (according to the answer reposted by @louisabraham ) |
Also, I have access to the Windows platform. Let me know what you need to be tested. |
We will allow |
Perfect, thank you! @louisabraham has explained the same thing! Does this merge close your issue #31 ? |
Are the wheels updated in the repo? Because returning to my original issue #30, the error in my setup is the same with the latest wheels found by pip:
|
Yes. Manually closed now. Thank you!
We have limited this PR for migrating from TravisCI to Github Actions for building our wheels. The final step of uploading the wheels/sdist to PyPI is being tracked in #37. To answer your question, the new wheels are not available on PyPI yet. |
I have to configure the uploading to PyPI myself. I'll try to find time for this over the WE. |
Work-In-Progress. Do not merge!
This is not meant to be comprehensive and takes an initial stab at addressing #31
The Github Actions workflow that is currently being triggered can be viewed here.
TO DO