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

Update matrix os & python-version of CI workflow #263

Closed
wants to merge 5 commits into from

Conversation

FarnazH
Copy link
Member

@FarnazH FarnazH commented Apr 7, 2021

The CI workflow's matrix is updated to test all various versions of Python for various operating systems (including windows). Only a subset of these combinations was used before. Is this useful @tovrstra?

Now I see all the Window's tests failing. Does the ci.yml need to be updated?

@FarnazH FarnazH requested a review from tovrstra April 7, 2021 01:16
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #263 (c64f838) into master (333126a) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   95.64%   95.62%   -0.02%     
==========================================
  Files          74       74              
  Lines        8214     8214              
  Branches     1068     1068              
==========================================
- Hits         7856     7855       -1     
- Misses        164      165       +1     
  Partials      194      194              
Impacted Files Coverage Δ
iodata/test/test_fchk.py 99.79% <0.00%> (-0.21%) ⬇️

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 333126a...c64f838. Read the comment docs.

@tovrstra
Copy link
Member

The tests on Windows don't work yet because I used bash scripts for most commands, while windows uses Powershell by default. I'll try a simple fix. We may run into the same issue when subprocesses are launched, which require bash instead of pwsh.

I did not include the other python version on Mac because it seemed redundant. If there is some added value, I wouldn't mind taking all combinations.

@FarnazH Would we be fine with dropping support for Python 3.6? I would not mind at all. It would allow is to strip away some ugly imports.

@tovrstra
Copy link
Member

That helped somewhat. As expected, subshells still default to powershell, while bash assumed. There also seems to be a caching issue, but I'm not sure why that happens.

@tovrstra
Copy link
Member

This will require a bit more work on other repositories (Roberto mainly) to work, but I see how it can be fixed relatively easily. The main remaining issue is the usage of forward slashes in some paths. Most of these should be easy to fix.

We probably have to disable caching if we want to test on all combinations of OSes and Python versions. I'm fine with that. Caching resulted in a small speedup for the venv test. It mainly saves time for testing in conda, which is only done on the main branch after merging.

I'll revisit this when external issues are resolved.

@FanwangM
Copy link
Contributor

FanwangM commented Jan 3, 2023

If we need to use bash in windows, would using a WSL docker image for GitHub Actions be an option? Or use https://github.com/marketplace/actions/setup-wsl?

@FarnazH @tovrstra @PaulWAyers

@tovrstra
Copy link
Member

tovrstra commented May 6, 2023

I get the impression WSL is gaining widespread adoption among Windows users who what to do sth unix-y, making native support for Windows less relevant.

@FanwangM To what extent is it useful to use WSL in GitHub Actions? As far as I understand, WSL is just a way to run Ubuntu in Windows. That would mean testing on Ubuntu is sufficient to guarantee that things work on WSL too?

@PaulWAyers
Copy link
Member

WSL is Ubuntu by default but any distribution can be used. That said, I tend to use WSL only when it's required (which is rare nowadays).

@tovrstra
Copy link
Member

tovrstra commented May 9, 2023

I'm fine with native Windows support as a final ambition. (There are a few hurdles before we'll get there.) I'd like to propose structural changes to make this (and CI and QA in general) a little easier.

I've been testing new development tools recently and was pleasantly surprised by how things have evolved in the last few years and months. Most of the things I felt were missing (back in the days when I wrote Roberto and Cardboardlint) have become available in fairly standard tools nowadays. Just to show this with a practical example, I've reorganized the denspart repository with the following tools:

  • pre-commit.com and pre-commit.ci to bundle multiple linting tools and to run them at the right time (locally and in the cloud). I use this instead of cardboardlint. It also covers many of the features of tox in a more general framework.
  • ruff looks like it will become the future of Python linting (replacing pylint, flake8, pydocstyle, ...) Ruff is still young, but it is insanely fast and good. Try it once, and you will not look back.
  • Tests on different Python (and OS) versions run on GitHub Actions. I just have one (recent) Python version locally for testing. (I'm using direnv for switching quickly between environments of different projects.) I'm too impatient to run tests on different python versions locally. It takes too long, and then it is still limited to only one operating system. This discouraged me further from using tox (or poetry).
  • Replace setup.py by pyproject.toml, which practically means the packages is placed under src as is the case for grid. (This is specific for setuptools.) I appreciate the advantages of the src directory more than I used to. This blog post is quite insightful, despite its age.
  • Configure editor settings in an editorconfig file. These will be picked up by most IDEs, helping consistency, even when we all use different editors.
  • Version numbers can be derived dynamically from Git tags with standard, e.g. with setuptools_scm.
  • Automatic releases to PyPI and documentation builds work well. It's not worth the trouble anymore to develop custom tools.
  • I stopped building conda packages in the workflow. It is quite cumbersome because conda build is slow. At the same time conda-forge is capable of doing all that hard work.
  • I write the changelog using the following conventions: https://keepachangelog.com/
  • (I still need to check out dependabot.)

@PaulWAyers
Copy link
Member

I'm 100% in favor of using these new tools for projects going forward. It may help with the "bus factor" a bit too.

If you are looking for a package under (very) rapid development to test out, grid is going fast right now, thanks to @Ali-Tehrani and @FarnazH .

If you write very detailed instructions on how to set things up, I can try to set it up myself too, just to make sure it's not too complicated. The other thing to think about is whether we should make a QC-Devs template repo that we use when we start new repositories, which would have some of this semi-set-up.

@tovrstra
Copy link
Member

tovrstra commented May 9, 2023

Sounds good. To make the switch, it's good to have all lingering branches and pull requests merged, because some changes can be invasive and cause merge conflicts with unmerged changes. I don't have a clear list of instructions at this stage. These depend a bit on the starting point. For grid, there would be fewer changes required than for IOData. (I'll add a few items to the list above, to have at least something more detailed.)

A template repository would make sense. The following could be a source of inspiration, but is also a bit dated and contains things less relevant to us: cookiecutter-pyproject. In general, the cookiecutter is a good, generic and independent tool for building templates. I've used it for courses and now also for manuscripts.

@tovrstra
Copy link
Member

I'm closing this pull request because it is fixed slightly differently in #312. Follow-up discussions can take place in #313.

@tovrstra tovrstra closed this May 14, 2024
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.

4 participants