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!: drop python 3.6 support #2247

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented May 27, 2023

Update setup.py minimum python version requirement.
Update workflows to use Python 3.9 by default.
Update container image docs.

@swcurran @WadeBarnes I believe these are the relevant changes in workflows and setup files in order to officially drop Python 3.6. I think once we've made the transition, there are a number of dependencies that ought to be updated. I think those updates are best suited to a follow up PR.

closes: #2244

@dbluhm
Copy link
Contributor Author

dbluhm commented May 27, 2023

The integration test action used von-image as a base which had python 3.6 installed. I switched the base image to a locally built indy base image (indy-base target in Dockerfile.indy). However, this is not currently caching the image build so this adds a significant amount of time to the action (since building Indy takes a while). We could switch to running the integration tests on Askar (maybe it already is?) to reduce build time in the integration test action.

@swcurran
Copy link
Contributor

Could we not add a new von-image that is based on Python 3.9 so everything is essentially the same? Definitely think we should be use Askar as the default.

@swcurran
Copy link
Contributor

LGTM, but deferring to @WadeBarnes for a final answer and merge.

@dbluhm
Copy link
Contributor Author

dbluhm commented May 30, 2023

Integration tests didn't finish cleanly -- I'll have to take a closer look

@dbluhm
Copy link
Contributor Author

dbluhm commented May 30, 2023

I'll try to keep this timely but if someone beats me to it, contributions are welcome

Copy link
Contributor

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

Some minor recommendations

.github/workflows/nightly-tests.yml Outdated Show resolved Hide resolved
.github/workflows/nightly-tests.yml Outdated Show resolved Hide resolved
docker/Dockerfile.test Outdated Show resolved Hide resolved
ContainerImagesAndGithubActions.md Outdated Show resolved Hide resolved
@WadeBarnes
Copy link
Contributor

Once the integration tests are working, I'll have to switch over the Python 3.9 Tests over to the Required ones:
image

@WadeBarnes
Copy link
Contributor

The integration tests are running through to completion locally for me. It seems like they are getting hung up somewhere in GHA.

@swcurran
Copy link
Contributor

So good to merge then, @WadeBarnes ?

@swcurran
Copy link
Contributor

@dbluhm — can you update the branch so we can merge?

@WadeBarnes
Copy link
Contributor

So good to merge then, @WadeBarnes ?

@swcurran , Not yet, there are some changes that need to be made. @dbluhm, I can make the changes if you agree to them.

@dbluhm
Copy link
Contributor Author

dbluhm commented May 30, 2023

I agree with the recommended changes. I wondered if it made sense to continue to test against 3.7 and 3.8 but given that I bumped the minimum version to 3.9, that wouldn't really make sense.

@dbluhm
Copy link
Contributor Author

dbluhm commented May 30, 2023

@WadeBarnes if we update lines like this one in the demo, we could probably change the base from von-image or indy-base to just a standard python image. This would remove the opportunity for running with Indy in the demo, though, since the library wouldn't be available in the image.

@swcurran
Copy link
Contributor

Who’s got the ball on this one? I’d really like to get this merged...

@WadeBarnes
Copy link
Contributor

In my court. I'll get things done first thing in the morning.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Jun 1, 2023

@dbluhm, I've made the changes, however I'm unable to push them to the Indicio-tech:chore/drop-py3.6 branch. It seems the permission for maintainers to edit was not set when the PR was created. I can PR to Indicio-tech:chore/drop-py3.6 directly if needed. Let me know.

The changes are currently sitting here: WadeBarnes:chore/drop-py3.6

@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 1, 2023

@WadeBarnes I merged your branch locally and pushed but I'll see if I can push the right buttons to allow edits from maintainers in case any other changes are needed. It might be because I opened it from Indicio's fork and permissions on our org, not sure.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Jun 1, 2023

@WadeBarnes I merged your branch locally and pushed but I'll see if I can push the right buttons to allow edits from maintainers in case any other changes are needed. It might be because I opened it from Indicio's fork and permissions on our org, not sure.

Thanks, You should see a checkbox on the PR for the edit option. Example:
image

@WadeBarnes
Copy link
Contributor

Lots of timeout errors in the integration tests. I'll have to look into those.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 1, 2023

image
No such box, unfortunately 😞 I suspect it is an org permissions thing. I'm happy to close this PR in favor of one you open. Also happy to merge in changes as before. Whatever is most convenient for you!

@WadeBarnes
Copy link
Contributor

Lets figure out the issue with the test first and then decide how to proceed.

@WadeBarnes
Copy link
Contributor

Quick update on the integration test issue. It's definitely something to do with the differences in the base images used for the bdd tests. When using the original base image the tests run to completion. When using the updates that use the indy-base image there are timeout errors. What's strange is the issues only occur on GHA, when the same setup is used locally on an Ubuntu devContainer the tests run just fine.

Run using the original base image setup:
https://github.com/WadeBarnes/aries-cloudagent-python/actions/runs/5147239795

Runs using the updated base image setup:
https://github.com/WadeBarnes/aries-cloudagent-python/actions/runs/5146808656/jobs/9266369701

  • Using this setup, the tests run locally without issue.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Jun 1, 2023

Suspecting a startup timing issue. Testing that theory.

Edit: That's not it

@WadeBarnes
Copy link
Contributor

The issues are related to the Python version of the base, indy-base container. I've tested with 3.6, 3.7, 3.8, and 3.9. The crashing and hanging issues start occurring with python 3.8 on GHA, yet locally using a docker-in-docker devContainer I can run the tests successfully with python 3.8 and 3.9. With the @T001-RFC0160 tests the Mediator.Agent.8035 agent fails to start in a timely fashion, errors occur and the tests end up hanging.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Jun 8, 2023

I need a little help with this issue. I'm not having much luck tracking down why the tests will run locally but not in GHA. My most recent run of the tests with logging set to info is here; https://github.com/WadeBarnes/aries-cloudagent-python/actions/runs/5214202473/jobs/9410171574. In the scenario were a mediator is created for both Acme and Bob, Bob's mediator never starts, and there are no logs to indicate any possible errors. There is also this run with logging set to debug https://github.com/WadeBarnes/aries-cloudagent-python/actions/runs/5213743145/jobs/9409134016.

I'm hope I'm missing something simple.

I've limited the testing to -t @T001-RFC0160 since that reproduces the issue. Locally that looks like LEDGER_URL=http://test.bcovrin.vonx.io PUBLIC_TAILS_URL=https://tails-test.vonx.io LOG_LEVEL=info NO_TTY="1" ./run_bdd -t @T001-RFC0160

I've added the devContainer I'm using for local testing to the troubleshoot-integration-tests of my fork.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 8, 2023

I'll see if I can squeeze in some time to analyze!

@dbluhm dbluhm force-pushed the chore/drop-py3.6 branch from 2489de3 to c9b47d0 Compare June 22, 2023 21:40
@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 22, 2023

I attempted increasing timeouts on the off chance it simply wasn't ready in time and observed the same failures. I've reverted the timeout change.

dbluhm and others added 5 commits June 28, 2023 12:33
Update setup.py minimum python version requirement.
Update workflows to use Python 3.9 by default.
Update container image docs.

Signed-off-by: Daniel Bluhm <[email protected]>
- Update workflows, remove support for Python 3.7 and 3.8
- Update Dockerfiles to use `py39` images.
- Update docs

Signed-off-by: Wade Barnes <[email protected]>
Co-authored-by: Darko Kulic <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm force-pushed the chore/drop-py3.6 branch from 0c51285 to 5808182 Compare June 28, 2023 16:33
@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 28, 2023

For posterity's sake, the issue was resolved by increasing the number of workers on the thread pool used to spawn the subprocesses for the mediators in the integration tests. On github action runners, the default pool size was 10 (number of processors * 5). This default changed in 3.8 and witnessed in our tests in 3.9:

Changed in version 3.8: Default value of max_workers is changed to min(32, os.cpu_count() + 4). This default value preserves at least 5 workers for I/O bound tasks. It utilizes at most 32 CPU cores for CPU bound tasks which release the GIL. And it avoids using very large resources implicitly on many-core machines.

Found here: https://docs.python.org/3.9/library/concurrent.futures.html#concurrent.futures.ThreadPoolExecutor

So max number of workers dropped from 10 to 6 by default, preventing the mediator process from spawning because the pool was full.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 28, 2023

@swcurran What is the order of operations required to get this merged? We need to finalize 0.8.2, right?

@WadeBarnes
Copy link
Contributor

@swcurran, has requested this be merged after the pending release. I'll make the updates to the required checks right before this PR is merged, otherwise it will adversely affect other pending PRs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

LGTM

@WadeBarnes
Copy link
Contributor

Once the tests are complete I just need to adjust the required status checks before merging.

@swcurran swcurran enabled auto-merge July 5, 2023 13:56
@swcurran swcurran merged commit dceb9e4 into openwallet-foundation:main Jul 5, 2023
@WadeBarnes
Copy link
Contributor

Required checks have been updated to
image

@dbluhm dbluhm deleted the chore/drop-py3.6 branch July 7, 2023 13:41
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.

Drop support for Python 3.6
5 participants