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 to CI #110

Merged
merged 16 commits into from
Nov 9, 2022
Merged

Update to CI #110

merged 16 commits into from
Nov 9, 2022

Conversation

jacobcook1995
Copy link
Collaborator

Description

I've updated a number of things about the CI setup. Let me know if any of them don't seem sensible.

  1. I've added python version 3.11 to the CI, as it is now out and we intend to support it.
  2. I've updated the poetry version used to v1.2.2, so that poetry.lock's created using poetry v1.2 don't cause CI build failures.
  3. Increased minimum mypy requirement (in pyproject.toml) from 0.961 to 0.981, as this version fixes my-py issue #13627. This means that it is no longer necessary to pin the CI to use python version 3.10.6, so 3.10 can be used instead.
  4. Updated actions/checkout and pre-commit/action to versions that use Node.js 16 rather than 12, see. This fixes the depreciation warning that was coming up for all successful action runs.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist

  • Make sure you've run the pre-commit checks: $ pre-commit run -a
  • All tests pass: $ poetry run pytest

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

Those all seem sound to me. One thing is that poetry 1.2 introduced a new way of grouping dependencies, so that you can specify extra dependencies in themed groups (so we could have a docs group and a testing group). That seems useful, so we might want to update pyproject.toml to use that.

@jacobcook1995
Copy link
Collaborator Author

Those all seem sound to me. One thing is that poetry 1.2 introduced a new way of grouping dependencies, so that you can specify extra dependencies in themed groups (so we could have a docs group and a testing group). That seems useful, so we might want to update pyproject.toml to use that.

Yeah that seems like a good idea, but probably only once everyone has updated to using poetry 1.2 (I didn't find updating all that straightforward). Should I make a separate issue for it?

@davidorme
Copy link
Collaborator

Personally, I think it makes sense to update it in this PR, then it has all moved across to poetry 1.2 (although I get that is backwards compatible). This is a tricky one - I'm not sure how this PR works across branches.

The poetry environment can only be updated by individuals manually, I think? But the pyproject.toml does specify the version required for building the package. What I don't know is whether that setting is only used to build actual distributions (wheels etc) or whether it also affects the 'development' installation that is running when we develop the code.

So if we merge this - do we then need to get the team to all update. And how does that play out for @dalonsoa and @alexdewar, who have other projects that might currently use older poetry.

Basically, I think we should do this but the results are not restricted to this repo?

@@ -19,7 +19,7 @@ jobs:
fail-fast: false
matrix:
os: [ ubuntu-latest, macos-latest, windows-latest ]
python-version: [ "3.9" , "3.10.6" ]
python-version: [ "3.9" , "3.10", "3.11" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Python 3.11? That's brave!

Not a bad idea to try with the latest version of Python, but just don't be surprised if things don't work yet. It'll probably be months (years?) before all the common packages actually work with v3.11.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh right, I think this is maybe a coming from Julia thing where the updates have often fixed major problems with the language, so people switch pretty quickly. I guess we can keep 3.11 in the CI for now and then remove it if/when it becomes a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I briefly added 3.11-dev (7c9ef44) and the wheels came off alarmingly quickly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, up to you. It's not a bad thing, but it just might mean you have to remove it from the CI again later -- not a big deal.

@alexdewar
Copy link
Collaborator

Don't worry about me and @dalonsoa. I think we'll probably be moving to poetry 1.2 in our own projects before long and we can always work around any problems.

The poetry devs have made the upgrade more painful than it needed to be, but I think it's worth doing in any case.

@jacobcook1995
Copy link
Collaborator Author

I've taken a shot at grouping the dependancies, let me know if they seem like sensible groupings. A fair number are ungrouped, but I guess groups will become more obvious as we develop more

@alexdewar
Copy link
Collaborator

alexdewar commented Nov 8, 2022

I've only ever bothered using a separate group for developer dependencies and maybe another one for building the docs. My logic has always been that you want all of your devs to run pre-commit and pytest etc. but you likely won't need tools for building docs as they'll just be running on the CI.

It's a personal choice though and the fact that poetry has added this capability suggests that other people want more granular groupings too.

Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

Just that final tweak to the poetry groups, I think, if that suggestion makes sense to you.

pyproject.toml Outdated
autodocsumm = "^0.2.8"
pydocstyle = "^6.1.1"

[tool.poetry.dev-dependencies]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we've got:

  • devenv, which is everything in precommit and the rogue dev-dependencies.
  • and then test and docs as you have them.

I don't think this makes a big difference, but those three seem like a sensible development base setup and then specific extra areas? We can delete doctestfn - that was used in the testing and training, which has moved out of this repo.

Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

LGTM

@jacobcook1995 jacobcook1995 merged commit 5567582 into develop Nov 9, 2022
@jacobcook1995 jacobcook1995 deleted the feature/ci_fix_and_update branch November 9, 2022 12:48
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.

Positional-only parameters with python 3.10.7 (and numpy) (fixed in 0.981)
3 participants