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

[MNT] new CI workflow to test extras #5375

Merged
merged 37 commits into from Oct 21, 2023

Conversation

yarnabrina
Copy link
Collaborator

@yarnabrina yarnabrina commented Oct 6, 2023

new CI workflow that tris to install sktime each extra to see if the specified packages (with specified versions or versions bounds) are compatible with one another.

when does it run:

  1. runs first of every month at 00:00
  2. runs for a PR into mainthat modifies pyproject.toml
  3. runs when triggered manually

what does it do:

  1. runs for all supported operating system
  2. runs for all supported python version
  3. runs for all supported extras
  4. creates a new python environment of specific python version on a specific operating system with a specific python version and installs sktime with a specific extra

what does it not do:

  1. installation multiple extras simultaneously - too many combinations
  2. automated resolution if installation fails with conflicts - probably impossible

other changes

  1. formatted of pyproject.toml with pyproject-fmt as needed to test workflow
  2. skipped mrsqm from classification extra as it was failing to be built in most combinations
  3. added python version bound for relevant packages

@yarnabrina yarnabrina added the maintenance Continuous integration, unit testing & package distribution label Oct 6, 2023
@yarnabrina yarnabrina marked this pull request as draft October 7, 2023 15:05
@yarnabrina yarnabrina marked this pull request as ready for review October 7, 2023 18:03
* origin/main:
  [MNT] skpro as a soft dependency (sktime#5273)
  [DOC] Correct code block formatting for pre-commit install command (sktime#5377)
  [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311)
  [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373)
  [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376)
  [DOC] `sktime` intro notebook (sktime#3793)
  [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353)
  [ENH] refactor parallelization backend calls in utility module (sktime#5268)
  [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326)
  [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical`  (sktime#4927)
  [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283)
  [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365)
  [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321)
* origin/main:
  [MNT] [Dependabot](deps-dev): Update arch requirement from <6.2.0,>=5.6.0 to >=5.6.0,<6.3.0 (sktime#5392)
  [BUG] `statsforecast 1.6.0` compatibility - fix argument differences between `sktime` and `statsforecast` (sktime#5393)
  [BUG] `statsforecast 1.6.0` compatibility - in `statsforecast` adapter, fixing `RuntimeError: dictionary changed size during iteration` (sktime#5317)
  [DOC] fix typo in classification notebook (sktime#5390)
  [DOC] fix broken docstring example of `AlignerDtwNumba` (sktime#5374)
  [ENH] incremental testing to also test if any parent class in sktime has changed (sktime#5379)
  [ENH] simplified delegator interface to `dtw-python` based dynamic time warping distances (sktime#5348)
  [ENH] `YfromX` - probabilistic forecasts (sktime#5271)
  [ENH] removed py37.dockerfile and update doc entry for CI (sktime#5356)
  [ENH] lucky dynamic time warping distance and aligner (sktime#5341)
  [ENH] sensible default `_get_distance_matrix` for time series aligners (sktime#5347)
  [ENH] delegator for pairwise time series distances and kernels (sktime#5340)
@yarnabrina
Copy link
Collaborator Author

I'm not sure if we are still in feature freeze or not, but is this ready to be merged? It does not affect any existing CI or functionality to the best of my knowledge.

Summary of changes:

  1. adds an action to install an extra to see if dependencies are compatible with one another
  2. adds an workflow based on above action to auto-trigger in case of change in pyproject.toml, and in a schedule
  3. formats pyproject.toml using https://pypi.org/project/pyproject-fmt/

CC: @fkiraly, @benHeid, @achieveordie

(Reason for this bump/reminder is that with every new change in main branch's pyproject.toml, Github decides it's a conflict and I've to solve manually. If I skip the changes in pyproject.toml, no more conflict problem but I can't test the workflow in this PR anymore.)

@yarnabrina yarnabrina marked this pull request as draft October 14, 2023 14:39
…date-extras-workflow

* origin/auto-format-pyproject: (27 commits)
  added new hook
  format using new hook
  added new hook
  [DOC] fixing docstring example for `FhPlexForecaster` (sktime#4931)
  Revert "[MNT] instant release action" (sktime#5421)
  [MNT] instant release action (sktime#5419)
  Release 0.24.0 (sktime#5403)
  🚀 python 3.12 🚀  (sktime#5345)
  Revert "[MNT] instant release action" (sktime#5415)
  [MNT] 0.24.0 deprecations and change actions (sktime#5404)
  [MNT] [Dependabot](deps-dev): Update holidays requirement from <0.34,>=0.29 to >=0.29,<0.35 (sktime#5342)
  [MNT] Migrate from `pykalman` to `pykalman-bardo` (sktime#5277)
  [MNT] [Dependabot](deps-dev): Update skpro requirement from <2.1.0,>=2.0.0 to >=2.0.0,<2.2.0 (sktime#5396)
  [MNT] [Dependabot](deps-dev): Update numba requirement from <0.58,>=0.53 to >=0.53,<0.59 (sktime#5319)
  [MNT] update numba requirement from <0.58,>=0.53 to >=0.53,<0.59"" (sktime#5299)
  [MNT] instant release action (sktime#5265)
  Release 0.23.1 - omitted changelog commit (sktime#5413)
  Release 0.23.1 (sktime#5402)
  [ENH] Refactor of `DateTimeFeatures` tests to `pytest` fixtures (sktime#5397)
  [ENH] remove legacy except in `TestAllEstimators` for `predict_proba` (sktime#5386)
  ...
@yarnabrina yarnabrina marked this pull request as ready for review October 15, 2023 05:08
@fkiraly
Copy link
Collaborator

fkiraly commented Oct 15, 2023

not a review yet, but some questions:

  • there are 160 CI (matrix) elements here - can you explain how these arise?
  • the title of the CI element is quite long, so one can't see the total runtime or status string. Perhaps we should shorten it?
    • perhaps just "extras", since it's clear that it's about sktime and it's clear that it's a test...

@yarnabrina
Copy link
Collaborator Author

the title of the CI element is quite long, so one can't see the total runtime or status string

Not following this question. Can it be the case that it depends on screen size or anything? I'm not sure what can't be seen. I shortened th strings nevertheless.

there are 160 CI (matrix) elements here - can you explain how these arise?

As part of #5136, I introduced 9 new extras. This workflow tries to test each of these on each supported python version (3.8 to 3.12 - so 5 each) on each supported operating system (so 3 each) - leading to 135 jobs. The rest of the CI jobs are from existing workflows, as those are completely unchanged so far.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 15, 2023

Can it be the case that it depends on screen size or anything? I'm not sure what can't be seen. I shortened th strings nevertheless.

Yes, of course - we need to think of people with normal scree sizes, too...

Also I think the names of the workflow elements should be self-descriptive to any new contributor:
image
I think "extras" is too contextual, although I have no good alternative suggestion.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 15, 2023

This workflow tries to test each of these on each supported python version (3.8 to 3.12 - so 5 each) on each supported operating system (so 3 each) - leading to 135 jobs

That's 160 minutes CI time - are you saying, these run even if there is no change to the modules?

That's ca 2/3 of the current total CI time of a minimal PR, so would this then roughly doule expected CI time caused y the average PR?

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 15, 2023

when does it run:

Oh, I see, it does not run for the average PR, right?

@yarnabrina
Copy link
Collaborator Author

Oh, I see, it does not run for the average PR, right?

Yes, it does not. Only for PR's modifying the pyproject.toml.


Is it okay to merge this PR now? I think the timeout/connection errors are out of scope of this PR?

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 15, 2023

Is it okay to merge this PR now? I think the timeout/connection errors are out of scope of this PR?

I won't block it or approve it until I fully understand it, so you are fine to merge it as there is no change request (for 1 week), and the change is reversible (so the lazy approval process applies).

@yarnabrina
Copy link
Collaborator Author

yarnabrina commented Oct 15, 2023

so you are fine to merge

I don't have sufficient rights to merge without approval. So, keeping it open till you feel okay to merge all CI PR's. There'll be one more PR for dataset testing to be added in this week.

This is probably my desired merge order:

  1. this one ([MNT] new CI workflow to test extras #5375)
  2. split CI plus per version code coverage ([MNT] Split CI jobs per components with specific soft-dependencies #5304)
  3. diagnostic for base changes ([MNT] fix typos in base module #5313)
  4. diagnostic for forecasting changes ([MNT] fix typos in forecasting module #5314)
  5. code quality ([MNT] adds code quality checks without outdated/deprecated Github actions #5427)
  6. scheduled dataset workflow ([MNT] Dataset downloader testing workflow #5437)

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks, reviewed and approved.

Number 1 in the sequence of merges.

@fkiraly fkiraly merged commit 5d729c4 into sktime:main Oct 21, 2023
162 of 163 checks passed
fkiraly pushed a commit that referenced this pull request Oct 21, 2023
…5304)

Part 2, 3 and 4 of #5101
Depends on #5375

1. new extra with just test dependencies
2. action to run tests in `sktime/base` with only core and test
dependencies
3. action to run tests in `sktime/<component>` with only core, test and
component specific dependencies
4. workflow to run base framework tests only when `sktime/base` gets
modified
5. workflow to run component specific tests only when
`sktime/<component>` gets modified
6. workflow to run all (base framework and all components) tests every
SUnday 00:00 and manually if desired
@yarnabrina yarnabrina deleted the validate-extras-workflow branch October 22, 2023 03:42
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 22, 2023
* origin/main:
  [BUG] Fix pandas `FutureWarning` for silent upcasting (sktime#5395)
  [BUG] CLASP logic: remove indexes from exclusion zone that are out of range (sktime#5459)
  [MNT] Split CI jobs per components with specific soft-dependencies (sktime#5304)
  [MNT] new CI workflow to test extras (sktime#5375)
  [MNT] xfail remote data loaders to silence sporadic failures (sktime#5461)
  [BUG] [MNT] removed two recently added hooks (sktime#5453)
  [MNT] bound `pycatch22<0.4.4` due to breaking change in patch version (sktime#5434)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 22, 2023
…sting

* origin/main:
  [BUG] Fix pandas `FutureWarning` for silent upcasting (sktime#5395)
  [BUG] CLASP logic: remove indexes from exclusion zone that are out of range (sktime#5459)
  [MNT] Split CI jobs per components with specific soft-dependencies (sktime#5304)
  [MNT] new CI workflow to test extras (sktime#5375)
  [MNT] xfail remote data loaders to silence sporadic failures (sktime#5461)
  [BUG] [MNT] removed two recently added hooks (sktime#5453)
  [MNT] bound `pycatch22<0.4.4` due to breaking change in patch version (sktime#5434)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 22, 2023
…heck

* origin/main:
  [BUG] Fix pandas `FutureWarning` for silent upcasting (sktime#5395)
  [BUG] CLASP logic: remove indexes from exclusion zone that are out of range (sktime#5459)
  [MNT] Split CI jobs per components with specific soft-dependencies (sktime#5304)
  [MNT] new CI workflow to test extras (sktime#5375)
  [MNT] xfail remote data loaders to silence sporadic failures (sktime#5461)
  [BUG] [MNT] removed two recently added hooks (sktime#5453)
  [MNT] bound `pycatch22<0.4.4` due to breaking change in patch version (sktime#5434)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 22, 2023
* origin/main:
  [BUG] Fix pandas `FutureWarning` for silent upcasting (sktime#5395)
  [BUG] CLASP logic: remove indexes from exclusion zone that are out of range (sktime#5459)
  [MNT] Split CI jobs per components with specific soft-dependencies (sktime#5304)
  [MNT] new CI workflow to test extras (sktime#5375)
  [MNT] xfail remote data loaders to silence sporadic failures (sktime#5461)
  [BUG] [MNT] removed two recently added hooks (sktime#5453)
  [MNT] bound `pycatch22<0.4.4` due to breaking change in patch version (sktime#5434)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants