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

Feat: tox implementation #1989

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Feat: tox implementation #1989

wants to merge 3 commits into from

Conversation

moe-ad
Copy link
Contributor

@moe-ad moe-ad commented Jan 6, 2025

This PR is related to #296.
This is not a catch all PR, the idea is to first migrate some CI workflows to tox and gradually extend to other CI workflows via other PRs.

@moe-ad moe-ad self-assigned this Jan 6, 2025
@moe-ad
Copy link
Contributor Author

moe-ad commented Jan 6, 2025

PyDPF uses a custom wheel building step in CI. The first commit implements a way of integrating that step with tox.

@PProfizi
Copy link
Contributor

PProfizi commented Jan 6, 2025

Hi @moe-ad, thanks for this work.
Could we get a quick reminder of the advantages of switching to Tox?
I do not see it described here https://dev.docs.pyansys.com/how-to/testing.html

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (e6e73e8) to head (78fb2cd).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1989      +/-   ##
==========================================
- Coverage   88.44%   87.25%   -1.20%     
==========================================
  Files          89       89              
  Lines       10251    10251              
==========================================
- Hits         9066     8944     -122     
- Misses       1185     1307     +122     

@moe-ad
Copy link
Contributor Author

moe-ad commented Jan 6, 2025

@PProfizi
The immediately advantages would be:

  • Having workflows that can run locally similarly to CI runs.
  • Centralizing CI patch scripts. If the logic in these scripts can be specified entirely within tox.ini, they can be removed altogether.
  • Removing friction for contributors: easier to specify instructions like "install tox, then run tox xx to build documentation or tox yy to run tests" than requiring understanding of the entire repo layout to get testing/doc-build tasks done.
  • Isolate test environments: this is particularly useful for PyDPF's use case. Per test set in CI, a different environment will be spin up without any interference with/from the environment of other test sets.
  • Tox's invocation syntax is very flexible which is especially useful for local runs. Tests can be chosen to run sequentially or in parallel, or even running one specific test / group of tests. The same configuration allows this flexibility.
  • Of course being able to run in parallel should ideally translate to faster test runs.

The long term advantages would be:

  • Easy invocation of workflows across multiple python versions: currently most testing/doc-build is on py39. If we do decide to extend these workflows to more python versions, tox makes this easy to do.
  • The point above doesn't apply to python version alone, we can spin up test environments for any combination of dependencies we wish to execute a workflow against.

Those are the ones I can think of at the moment. I remember @jorgepiloto also shared many advantages. He will be back by tomorrow and he can also provide his inputs.

@moe-ad
Copy link
Contributor Author

moe-ad commented Jan 7, 2025

@PProfizi
Now simply running locally:

  • tox -e runtests-parallel should run all tests in parallel
  • tox -e runtests-sequential should run all tests sequentially

With automated test target organization and server process(es) shutdown before and after all tests have run.

Per test (irrespective of the running mode), a fresh environment with individual ansys.dpf.core package installation is used for test runs. The sacrifice with running in parallel is not having detailed output to stdout, only success/failure summary is shown for each test environment. Possible to force detailed output to stdout but that will result in an unstructured report. Maybe there is some log file being generated that can be inspected at the end of all runs?

Next steps:

  • Confirm all environment variables we need to be passing to the test environment since tox won't automatically pass them.
  • The package building step is done for each test environment. Will be nice if it can be done just once, and then installed into each environment as needed. Tox is smart enough to do this if package building were via a pyproject.toml or something similar.

Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @moe-ad. I know it is still in draft mode, but wanted to raise a point about auto-detecting the platform.

Comment on lines +25 to +44
[testenv:build_external]
description = Environment for custom build of package wheels, solves PyDPF custom wheel building requirement

allowlist_externals =
bash

package_glob = {toxinidir}{/}dist{/}ansys_dpf_core*

commands =
# Build the wheel
bash -c '\
if [ {on_platform} == "linux" ]; then \
export platform="manylinux_2_17"; \
elif [ {on_platform} == "win32" ]; then \
export platform="win"; \
else \
echo "Unknown OS"; \
fi; \
echo $platform; \
python .ci/build_wheel.py -p $platform -w'
Copy link
Member

Choose a reason for hiding this comment

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

Tox is able to detect the platform it is running. Just use the following:

platform =
    linux: linux
    windows: win32

From then one, you can execute commands based on this platform by using this form:

commands =
    linux: ls
    windows: dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgepiloto I have been thinking about this for a while. I have had another go at it after reading your comments without success.

It won't work via the above suggestions, at least based on what I have tested. tox expects any factor being used for conditional settings to be present in the environment name itself, otherwise the commands linked to the linux/windows factors will never be executed when tox encounters them. At the same time, the naming specification of an external wheel building environment must have the format <package_env>_external, meaning we can't rename the wheel building environment to [<package_env>-{linux,windows}_external] or something similar, and because of that the commands are not executed when tox encounters them. Maybe there is something I am missing?

All of these complications can be significantly reduced if there is a way to specify the wheel building logic directly within pyproject.toml. Being able to do this will also simplify some other things. @PProfizi was asking if this is possible yesterday, any ideas?

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