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

GA: Add pypy3 #972

Merged
merged 3 commits into from
Feb 19, 2021
Merged

GA: Add pypy3 #972

merged 3 commits into from
Feb 19, 2021

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 2, 2021

No description provided.

@rht
Copy link
Contributor Author

rht commented Jan 2, 2021

Failed to install black on pypy3.

@rht
Copy link
Contributor Author

rht commented Jan 2, 2021

psf/black#727

@Corvince
Copy link
Contributor

Corvince commented Jan 2, 2021

sigh seems like it will take a bit until black is supported. Not sure if its worth the hassle to set up an action that just runs pylint without black on pypy, but if you @rht or someone else gives it a shot I think it would be worth to add.

@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #972 (884b109) into master (299485e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #972   +/-   ##
=======================================
  Coverage   88.94%   88.94%           
=======================================
  Files          19       19           
  Lines        1140     1140           
  Branches      192      192           
=======================================
  Hits         1014     1014           
  Misses         92       92           
  Partials       34       34           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 299485e...0ec8345. Read the comment docs.

@rht
Copy link
Contributor Author

rht commented Jan 2, 2021

Black is not the only problem. I used the older version of Black as per psf/black#727 (comment). Then the CI seemed to be stuck at installing Pandas.

@rht
Copy link
Contributor Author

rht commented Jan 2, 2021

Wow, it succeeded. It was slow to install the dependencies (~12min) but not more than that. If we enable caching I think adding pypy3 to CI will be feasible.

@jackiekazil
Copy link
Member

@rht is there another PR for enabling caching? (Would like to merge this.)

@rht
Copy link
Contributor Author

rht commented Jan 15, 2021

There is some complication with caching. Since there is no requirements.txt, then setup.py has to be hashed. As long as setup.py isn't changed, the cache persists. This means that we are no longer using the latest versions of the dependencies anymore. Most libraries pin their requirements to specific versions, and update them periodically, but it's not the case with Mesa.

(For reference, this is an example of caching: https://github.com/django-ftl/fluent-compiler/blob/a6048b856a3db0761ae05a9d3049489f6bb11cbe/.github/workflows/pythonpackage.yml#L29)

@Corvince
Copy link
Contributor

Mesa has a pipenv file we can use: https://github.com/actions/cache/blob/main/examples.md#python---pipenv

The pipfile.lock could use some love though... But we could set up dependabot to automatically create merge requests on new versions

@Corvince
Copy link
Contributor

Just as an idea we could run this in another github action, then we also don't need to use an old version of black.

@rht rht force-pushed the pypy branch 2 times, most recently from 823526f to ac7e536 Compare January 17, 2021 08:04
@rht
Copy link
Contributor Author

rht commented Jan 17, 2021

I added cache to this PR as a proof of concept. It worked. Dependencies install down to 43s from 12min.

Just as an idea we could run this in another github action, then we also don't need to use an old version of black.

Or put black installation in a separate step, and pin the version if matrix.python-version is pypy3.

@Corvince
Copy link
Contributor

Or put black installation in a separate step, and pin the version if matrix.python-version is pypy3.

Better yet: Run black only on a single python version, since the output for different python versions would be the same anyway. I can make that change once this is merged.

While this POC works, I think it will fail as soon as we add windows and macOS, since the pip cache directory would change. Maybe also run pypy on ubuntu, anyways? (You can resolve this either in this PR or the windws/macos one). As soon as you give feedback on this and delete the "testing whitespace" from the readme, I think this is good to merge.

@rht
Copy link
Contributor Author

rht commented Jan 19, 2021

Maybe also run pypy on ubuntu, anyways?

Did you mean "maybe only run pypy on ubuntu"?

@Corvince
Copy link
Contributor

Yes, sorry

@Corvince
Copy link
Contributor

@rht Do you want to update this PR since #970 is now merged or should someone else take over?

@rht
Copy link
Contributor Author

rht commented Feb 19, 2021

Updating it now.

rht added 3 commits February 18, 2021 19:12

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
@rht
Copy link
Contributor Author

rht commented Feb 19, 2021

Updated. The work to do Black only on a specific Python version can be done in a separate PR.

@Corvince Corvince merged commit 639bad5 into projectmesa:master Feb 19, 2021
@jackiekazil jackiekazil added this to the Oro Valley milestone May 21, 2021
@rht rht deleted the pypy branch April 5, 2022 22:26
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