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

Add Python 3.9 for unit tests in CI (experimental, do not stop other builds if it fails) #3854

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Oct 5, 2020

This is a small change with no associated Issue.

This is how GH actions handles experimental Java versions. I guess it should work with Python versions too…

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.
  • No dependency changes.

@kinow
Copy link
Member Author

kinow commented Oct 5, 2020

(Should fail for now as I think GH hasn't added support to 3.9 yet)

@kinow
Copy link
Member Author

kinow commented Oct 5, 2020

The bash container is failing to build due to… virtual memory I think?

2020-10-05T22:17:29.2744656Z    write (2, "virtual memory exhausted\n", 25);
2020-10-05T22:17:29.2744959Z    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2020-10-05T22:17:29.4147064Z �[0mgcc -c -g -O2 -Wno-parentheses -Wno-format-security -DHAVE_CONFIG_H  -I. -I../.. -I../.. -I../../lib -I. tparam.c
2020-10-05T22:17:29.4414545Z �[91mtparam.c: In function ‘memory_out’:
2020-10-05T22:17:29.4421362Z �[0m�[91mtparam.c:67:3: warning: implicit declaration of function ‘write’ [-Wimplicit-function-declaration]
2020-10-05T22:17:29.4422068Z    write (2, "virtual memory exhausted\n", 25);

@kinow kinow self-assigned this Oct 5, 2020
@kinow kinow added this to the cylc-8.0a3 milestone Oct 5, 2020
@kinow
Copy link
Member Author

kinow commented Oct 6, 2020

Ha! https://docs.docker.com/engine/reference/run/#runtime-constraints-on-resources

Interesting, I never had to limit memory utilization of containers. Today-I-Learned 🎉

@kinow kinow marked this pull request as ready for review October 6, 2020 01:39
@kinow kinow changed the title Add Python 3.9 in CI (experimental) Add Python 3.9 for unit tests in CI (experimental, do not stop other builds if it fails) Oct 6, 2020
docker build -t bash:test dockerfiles/bash/
docker run --name bash -v $(pwd -P):/root/cylc-flow --rm -t -d bash:test /bin/bash
docker build -t bash:test -m 1G --memory-swap 1G dockerfiles/bash/
docker run --name bash -v $(pwd -P):/root/cylc-flow --rm -t -d -m 1G --memory-swap 1G bash:test /bin/bash
Copy link
Member Author

Choose a reason for hiding this comment

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

Was getting some errors due to low virtual memory when building the container. Not sure if others have seen this, but might be good to constraint resources so our builds don't fail sporadically.

@kinow
Copy link
Member Author

kinow commented Oct 6, 2020

(low priority, happy to leave this one waiting until GH has 3.9 support if others prefer 👍 )

experimental: [false]
include:
- python-version: 3.9
experimental: true
Copy link
Member

Choose a reason for hiding this comment

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

Why are python-version and experimental lists above, but scalar values here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, hadn't noticed it. Both appear to work fine. But let me edit that commit to match the other values 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, just need to wait for CI to run and confirm it works too.

Copy link
Member

@hjoliver hjoliver Oct 6, 2020

Choose a reason for hiding this comment

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

(I see the GH Actions docs does it the way you originally did it, so maybe it works either way 🤞 )

Copy link
Member

Choose a reason for hiding this comment

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

Bugger:

Error: .github/workflows/test.yml (Line: 25, Col: 27): A sequence was not expected
Error: The template is not valid. .github/workflows/test.yml (Line: 25, Col: 27): A sequence was not expected

Copy link
Member Author

Choose a reason for hiding this comment

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

But good to know! I guess if you have multiple versions, then you will have to do something like

- python-version: 1
  experimental: true
- python-version: 2
  experimental: true
...

Copy link
Member

Choose a reason for hiding this comment

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

What does the "experimental" mean exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a feature in GitHub actions that allows to mark a branch of the matrix-tree as not-ready I guess? It's really common in Java and JavaScript, for new versions of the JVM or of Node (which are experimental in relation to the code; meaning that they may not work, and the developers/CI are "experimenting" with it? 😄 )

Copy link
Member

Choose a reason for hiding this comment

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

OK, fair enough. Just metadata then, no real consequences 👍

Copy link
Member

Choose a reason for hiding this comment

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

Can remove the experimental thing now 3.9 is out.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

The 3.9 block fails, as expected. Any opinions on whether or not we should merge like this or wait till GH-A adds 3.9 support? (It might look a bit odd to have expected test fails all the time?)

@kinow
Copy link
Member Author

kinow commented Oct 6, 2020

The 3.9 block fails, as expected. Any opinions on whether or not we should merge like this or wait till GH-A adds 3.9 support? (It might look a bit odd to have expected test fails all the time?)

We can review/approve and merge later.

They have some pull requests in GH actions virtual environments repo for Ubuntu 18.04 and 20.04, but the next updates don't seem to include Python 3.9. I think first it needs to make its way to Ubuntu package repositories, then they will create a PR to merge?

https://github.com/actions/virtual-environments/pulls

@hjoliver
Copy link
Member

hjoliver commented Oct 6, 2020

I'll label this as "blocked" for now, pending 3.9 on GH-A.

@hjoliver hjoliver added the BLOCKED This can't happen until something else does label Oct 6, 2020
@kinow
Copy link
Member Author

kinow commented Oct 6, 2020

Ignore last commits. I added one commit to test something I saw in GH actions docs for setup-python:

python-version: '3.9.0-alpha - 3.9.0' # SemVer's version range syntax

Didn't work 😬 reverted

@kinow kinow removed the BLOCKED This can't happen until something else does label Nov 4, 2020
@kinow
Copy link
Member Author

kinow commented Nov 4, 2020

I think the 3.9 unit tests will pass now, actions/setup-python#148

@kinow
Copy link
Member Author

kinow commented Nov 4, 2020

CI passed, two approvals. Merging 🎉

@kinow kinow merged commit f656c97 into cylc:master Nov 4, 2020
@kinow kinow deleted the py39-experimental branch November 4, 2020 10:24
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
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.

3 participants