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.

Sign up for GitHub

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

Jump to bottom

[MNT] fix several spelling mistakes #5639

Merged
merged 1 commit into from
Jan 6, 2024
Merged

[MNT] fix several spelling mistakes #5639

merged 1 commit into from
Jan 6, 2024

Conversation

yarnabrina
Copy link
Member

Diagnoses #5638

Depends on #5638

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@yarnabrina yarnabrina force-pushed the debug-ci-changes branch 3 times, most recently from f83bde4 to c089888 Compare December 19, 2023 12:31
@yarnabrina yarnabrina marked this pull request as ready for review December 19, 2023 14:06
This was referenced Dec 19, 2023
Merged
Open
@yarnabrina yarnabrina force-pushed the debug-ci-changes branch 3 times, most recently from 8bb02d1 to 1303464 Compare December 27, 2023 14:59
@yarnabrina yarnabrina mentioned this pull request Dec 28, 2023
Open
@yarnabrina
Copy link
Member Author

@fkiraly the failure is coming from a known bug I think (#5658), not from CI. Ignoring in this PR as of now.

FAILED sktime/tests/test_all_estimators.py::TestAllEstimators::test_multiprocessing_idempotent[TSFreshClassifier-ClassifierFitPredictMultivariate-predict] - ValueError: The feature names should match those that were passed during fit.
Feature names must be in the same order as they were in fit.

yarnabrina added a commit that referenced this pull request Dec 31, 2023
@yarnabrina
[MNT] refactoring new CI to fix some bugs and other minor enhancements (
2f62af5 
#5638)

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
Copy link
Member Author

@fkiraly rebased this PR with main after merging #5638. It just has fixes of several spelling mistakes, and no other change whatsoever.

@yarnabrina yarnabrina mentioned this pull request Dec 31, 2023
Open
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 1, 2024

... really hard to fid the single failing check among 380.

Is there a way to bundle these better?

@yarnabrina
Copy link
Member Author

This (ref. https://github.com/sktime/sktime/actions/runs/7370225847/job/20056856638) is the failure.

FAILED sktime/tests/test_all_estimators.py::TestAllEstimators::test_multiprocessing_idempotent[TSFreshClassifier-ClassifierFitPredictMultivariate-predict] - ValueError: The feature names should match those that were passed during fit.
Feature names must be in the same order as they were in fit.

In the CI summary page (ref. https://github.com/sktime/sktime/actions/runs/7370225847), it highlights what all failed. You need to go to Checks -> validate code quality workflow -> Annotations.

image

I am not aware of any other easier options.

@yarnabrina
Copy link
Member Author

@fkiraly after merging main to fetch #5673 changes, there's no CI failure any more. All changes for this PR are done from my side.

fkiraly
fkiraly approved these changes Jan 6, 2024
View reviewed changes
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! Fine in-principle, though I want to note:

the CI reportrs 1 out of 380 tests still queued, and I cannot find which one it is due to the list being so long. This is annoying, and we should think about reducing number of CI jobs spawned.

@yarnabrina
Copy link
Member Author

image

All checks have passed. Merging this, and we can discuss in #5706 on how to reduce number of jobs, or if it's possible.

@yarnabrina yarnabrina merged commit 3fc99f6 into sktime:main Jan 6, 2024
381 checks passed
@yarnabrina yarnabrina deleted the debug-ci-changes branch January 6, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fkiraly fkiraly fkiraly approved these changes

@achieveordie achieveordie Awaiting requested review from achieveordie

@benHeid benHeid Awaiting requested review from benHeid benHeid is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yarnabrina @fkiraly