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] refactoring new CI to fix some bugs and other minor enhancements #5638

Merged
merged 2 commits into from
Dec 31, 2023

Conversation

yarnabrina
Copy link
Collaborator

@yarnabrina yarnabrina commented Dec 19, 2023

Fixes #5559 (indirectly depends on #5670 for all tests to pass as part of #5639)

This fixes few bugs (typos, possibly unreliable coverage, etc.) and added some minor enhancements in new CI.

  1. fix main typo: sktime-component was using detect_components_with_extras which is undefined
  2. split test_components into two separate workflows:
    1. one to test individual components using dedicated extras and reuse defined actions
    2. one to test all other components without extras using all_extras_pandas2
  3. split long commands into multiple lines using YAML multiline strings (ref. https://yaml-multiline.info/)
  4. skip duplicated testing of base as part of testing components without dedicated extras
  5. remove coverage in partial tests
  6. add coverage in full tests if missing
  7. added validation of all_extras_pandas2
  8. minor changes (mostly renaming) in some places

The changes are tested in #5639 and as it can be seen in https://github.com/sktime/sktime/actions/runs/7339442367, both components with and without extras trigger sub-jobs as expected.

@yarnabrina yarnabrina changed the title [MNT] restore new CI behaviour by reverting changes added to close #5530 [MNT] fix typo in matrix specification Dec 19, 2023
@yarnabrina yarnabrina force-pushed the fix-new-ci branch 2 times, most recently from 6f53b13 to 570b329 Compare December 19, 2023 12:30
@yarnabrina yarnabrina marked this pull request as ready for review December 19, 2023 14:05
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.

I am confused with the PR title - it says it fixes typos, but there are entire files added in workflows - is this a mistake, or are file renames and changes happening in the same PR?

@yarnabrina yarnabrina changed the title [MNT] fix typo in matrix specification [MNT] refactoring new CI to fix some bugs and other minor enhancements Dec 21, 2023
@yarnabrina
Copy link
Collaborator Author

yarnabrina commented Dec 21, 2023

I am confused with the PR title - it says it fixes typos, but there are entire files added in workflows - is this a mistake, or are file renames and changes happening in the same PR?

The typo fix is the main change what was causing the issue that few tests were not running in new CI. However, fixing that led to copilot and github docs suggesting some other changes to simplify maintenance, so added those as well.

I renamed the PR now, but not sure if it's suitable. All the details are part of the description.

1. fix main typo: sktime-component was using detect_components_with_extras which is undefined
2. split test_components into two separate workflows:
    1. one to test individual components using dedicated extras and reuse defined actions
    2. one to test all other components without extras using all_extras_pandas2
3. split long commands into multiple lines using YAML multiline strings (ref. https://yaml-multiline.info/)
4. skip duplicated testing of base as part of testing components without dedicated extras
5. remove coverage in partial tests
6. add coverage in full tests if missing
7. added validation of all_extras_pandas2
8. minor changes (mostly renaming) in some places
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.

I'm happy with this after discussions on the discord channel.
The release cycle will also give us at least a few weeks to test this out.

We will still need to resolve:
outstanding questions on how and which parts of current CI to deprecate or replace, and
#5559
#5525
#5523

Related discussion also in:
#5671
#5436
#5477
#5292

@yarnabrina
Copy link
Collaborator Author

Notes/Summary

#5559 - addressed by this PR.
#5525 - not addressed yet, was attempted in #5498, but didn't work, reverted in 1dee835.
#5523 - not adressed yet, but it's more good-to-have than necessary in my opinion.

#5671 - partially addressed as pyproject changes would be tested, no explanation on dependabot's partial attempt.
#5436 - not addressed and out-of-scope for CI, but may be that issue can be broadened to even 3.12.
#5477 - not addressed, very good-to-have to decrease runtime of new CI, open for suggestions.
#5292 - not addressed, open for suggestions, indirectly can be tested by manual runs of test_all workflow on that branch.

@yarnabrina yarnabrina merged commit 2f62af5 into sktime:main Dec 31, 2023
189 checks passed
@yarnabrina yarnabrina deleted the fix-new-ci branch December 31, 2023 11:43
yarnabrina added a commit that referenced this pull request Jan 6, 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.

[BUG] behaviour of new CI changed suddenly
2 participants