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

chore: pin packages + docker pin-helper script - DROPS PY36 #175

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

hneiva
Copy link
Contributor

@hneiva hneiva commented Dec 15, 2022

Assumes sphinx will always run on python 3.9+ (as set in .readthedocs.yaml) Had to split into it's own requirements file due to conflict with upstream tox package pinning

@hneiva hneiva requested review from ahal, escapewindow and a team December 15, 2022 01:13
@hneiva hneiva force-pushed the hneiva/pin-packages branch 3 times, most recently from 8d134e6 to 590e3f7 Compare December 15, 2022 01:46
@hneiva
Copy link
Contributor Author

hneiva commented Dec 15, 2022

It looks like we'll have to either:
a. Drop py3.6 support
b. Manage requirement files versioning (can be done with docker) ie: base.py36.in etc

Side note that I had a lot of issues with sphinx and py3.6

@hneiva hneiva force-pushed the hneiva/pin-packages branch 5 times, most recently from ca5671f to 9b9d53e Compare December 17, 2022 00:19
tox.ini Outdated
@@ -9,7 +9,7 @@ depends =
report: py{37,38,39,310}
commands =
python --version
pytest -vv --cov=taskgraph --cov=src/taskgraph/run-task --cov-append --cov-report= {posargs}
pytest -n auto -vv --cov=taskgraph --cov=src/taskgraph/run-task --cov-append --cov-report= {posargs}
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'm not sold on the parallel test idea, but locally at least it saves multiple minutes per run, and doesn't seem to cause any issues. Thoughts?

Copy link
Contributor

@JohanLorenzo JohanLorenzo Dec 19, 2022

Choose a reason for hiding this comment

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

Fine by me! We can always revert this small change if we face too many intermittent errors.

Edit: I just noticed the perma-error on CI. Maybe it's not worth enabling it for now.

@hneiva
Copy link
Contributor Author

hneiva commented Dec 17, 2022

looks like coverage (required by pytest-cov) has been having issues with sql: nedbat/coveragepy#1303 and it's most frequest in github actions. I'm unable to reproduce the error locally

Copy link
Contributor

@JohanLorenzo JohanLorenzo left a comment

Choose a reason for hiding this comment

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

Fixes #137

Thanks a lot for looking into this issue. I'm in favor of dropping Python 3.6. I just checked what gecko uses and it's already on Python3.8[1]:

$ docker pull mozillareleases/gecko_decision:4.0.0@sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06
docker.io/mozillareleases/gecko_decision@sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06: Pulling from mozillareleases/gecko_decision
16ec32c2132b: Pull complete
1c6fb07fba89: Pull complete
e2066ab6a02e: Pull complete
08a664befb8a: Pull complete
Digest: sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06
Status: Downloaded newer image for mozillareleases/gecko_decision@sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06
docker.io/mozillareleases/gecko_decision:4.0.0@sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06


$ docker run -ti mozillareleases/gecko_decision:4.0.0@sha256:9f69fe08c28e3cb3cc296451f0a2735df6e25d0e3c877ea735ef1b7f0b345b06 bash
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
root@175b8e25177f:~# python --version
Python 2.7.18
root@175b8e25177f:~# python3 --version
Python 3.8.10

[1] https://searchfox.org/mozilla-central/rev/f40d29a11f2eb4685256b59934e637012ea6fb78/.taskcluster.yml#226

@@ -40,5 +40,5 @@ tasks:
using: run-task
cwd: '{checkout}'
command: >-
pip install --user -r requirements/test.txt &&
pip install --user -r requirements/test.txt --require-hashes &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Big 👍 for adding this param wherever it was missing! 💯

@@ -29,7 +29,7 @@ def hg_repo(tmpdir, monkeypatch):

# hg sometimes errors out with "nothing changed" even though the commit succeeded
subprocess.call(
["hg", "commit", "-m", "First commit", "--date", _FORCE_COMMIT_DATE_TIME],
["chg", "commit", "-m", "First commit", "--date", _FORCE_COMMIT_DATE_TIME],
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible typo: what is chg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, but I found a problem when running chg, I'll drop it for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a command server written in C that can bypass the startup latency caused by Python. Makes things a bit faster, though in this case I don't think it'll make a big difference vs the other improvements later on.

tox.ini Outdated
@@ -9,7 +9,7 @@ depends =
report: py{37,38,39,310}
commands =
python --version
pytest -vv --cov=taskgraph --cov=src/taskgraph/run-task --cov-append --cov-report= {posargs}
pytest -n auto -vv --cov=taskgraph --cov=src/taskgraph/run-task --cov-append --cov-report= {posargs}
Copy link
Contributor

@JohanLorenzo JohanLorenzo Dec 19, 2022

Choose a reason for hiding this comment

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

Fine by me! We can always revert this small change if we face too many intermittent errors.

Edit: I just noticed the perma-error on CI. Maybe it's not worth enabling it for now.

set -e
pip install -q -U pip pip-compile-multi
pip-compile-multi -u \
-g base -g test -g dev -g docs
Copy link
Contributor

@JohanLorenzo JohanLorenzo Dec 19, 2022

Choose a reason for hiding this comment

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

Nit: should we pin docs against python 3.9 instead of 3.7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial thought. But once I talked to ahal, we decided to go with the minimum supported version of python, as long as it causes no problem to the version that rtd is running.

Comment on lines +23 to +24
# Don't quit if a version fails
set +e
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the loop is to highlight failures in case the generated dependencies don't work on a later version of Python. If so, what's the use case for letting these commands silently fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved it now

@hneiva hneiva changed the title chore: pin packages + docker pin-helper script chore: pin packages + docker pin-helper script - DROPS PY36 Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Base: 63.88% // Head: 63.85% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (6a75841) compared to base (922b20d).
Patch has no changes to coverable lines.

❗ Current head 6a75841 differs from pull request most recent head 9b808ac. Consider uploading reports for the commit 9b808ac to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
- Coverage   63.88%   63.85%   -0.04%     
==========================================
  Files          64       64              
  Lines        5109     5107       -2     
==========================================
- Hits         3264     3261       -3     
- Misses       1845     1846       +1     
Impacted Files Coverage Δ
src/taskgraph/util/hash.py 79.16% <0.00%> (-1.61%) ⬇️
src/taskgraph/util/taskcluster.py 93.25% <0.00%> (-0.62%) ⬇️
src/taskgraph/util/path.py 90.78% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jcristau
Copy link
Contributor

My understanding was we're not ready to drop 3.6 because taskgraph is used (or at least imported) in parts of the firefox build system, and we don't want to break downstream builds. Did that change?

"""
The repo fixture depends on the session-scoped git and hg repo fixtures, and
copies the contents of the initialized repos to a temp folder
"""
Copy link
Contributor Author

@hneiva hneiva Dec 19, 2022

Choose a reason for hiding this comment

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

This should improve the reliability on the tests as well, as it reduces runtime by a few minutes, I'm hoping it's less likely for pytest to have issues with sqlite

@hneiva
Copy link
Contributor Author

hneiva commented Dec 19, 2022

My understanding was we're not ready to drop 3.6 because taskgraph is used (or at least imported) in parts of the firefox build system, and we don't want to break downstream builds. Did that change?

Aiui, as far as the requirements go, py36 isn't supported (even in main) because of tox, sphinx and others. That being said, we can still import and use taskgraph in py36.

@kenahoo
Copy link

kenahoo commented Dec 27, 2022

looks like coverage (required by pytest-cov) has been having issues with sql: nedbat/coveragepy#1303 and it's most frequest in github actions. I'm unable to reproduce the error locally

@hneiva I whipped up a sample repo that demonstrates the problem, at [nedbat/coveragepy#1514].

@ahal
Copy link
Collaborator

ahal commented Jan 3, 2023

Thanks for doing this! This lgtm, but you'll need to address the test failures before it can land. So requesting changes for now.

Copy link
Collaborator

@ahal ahal left a comment

Choose a reason for hiding this comment

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

Requesting changes until CI can be fixed.

Drops compiling requirements in py36 - taskgraph code is still py36 "compatible"
@hneiva hneiva merged commit 4035acf into main Jan 9, 2023
@hneiva hneiva deleted the hneiva/pin-packages branch January 9, 2023 15:15
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