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

&yarnabrina [MNT] testing only estimators from modules that have changed compared to main #5019

Merged
merged 13 commits into from
Aug 10, 2023
Merged

&yarnabrina [MNT] testing only estimators from modules that have changed compared to main #5019

merged 13 commits into from
Aug 10, 2023

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Aug 4, 2023 •
edited
Loading

Experimental PR that ensures only estimators are tested that have changed compared to main.

The design relies on a new utility that checks changes to main using subprocess calling git.

How we know it works:

  • The CI in this PR runs none of the suite tests for any of the estimators in sktime, since this PR does not change any of the relevant files.
  • whereas, in this PR DIAGNOSTIC [MNT] testing #5019 with dummy changes #5023 where two estimator modules have changed, it runs exactly the suite tests for the two estimators.

Also, this most likely gets us a good way towards #2890, at least when it comes to tests arising from the TestAll portion of the test framework.

@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution module:tests test framework functionality - only framework, excl specific tests labels Aug 4, 2023
@fkiraly fkiraly changed the title DRAFT [MNT] testing only estimators from modules that have changed compared to main. DRAFT [MNT] testing only estimators from modules that have changed compared to main Aug 4, 2023
@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 4, 2023

doesn't work like this, errors out with fatal: bad revision since it cannot see main.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 4, 2023

Update: this works locally, but not on remote - the reason being, inside the VM we apparently cannot see the main branch, only the feature/PR branch, so the comparison to main that would be required to identify the modules that have changed is not possible.

For this to work, we need to ensure that main is also available as a branch inside the VM. Help appreciated!

FYI @tarpas, any ideas?

@fkiraly fkiraly mentioned this pull request Aug 4, 2023
Open
@fkiraly fkiraly mentioned this pull request Aug 4, 2023
Closed
@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 4, 2023

This seems to be working!

Testing the positive case with #5023, i.e., the case that two estimator modules change. If that tests exactly those modules with the suite tests, we know it most likely works!

yarnabrina
yarnabrina reviewed Aug 4, 2023
View reviewed changes
Copy link
Member

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

Thank you so much for this PR. I'll be singificantly happier (at ANY level of significance) if the CI time decreases by running only what's necessary, at least for most of the times (except may be releases?).

I added some comments/doubts/suggestions.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
sktime/utils/git_diff.py Outdated Show resolved Hide resolved
sktime/utils/git_diff.py Outdated Show resolved Hide resolved
sktime/utils/git_diff.py Show resolved Hide resolved
fkiraly and others added 2 commits August 4, 2023 21:59
@fkiraly @yarnabrina
typo fix
7c59105 
Co-authored-by: Anirban Ray <39331844+yarnabrina@users.noreply.github.com>
@fkiraly @yarnabrina
raise from
184a5b8 
Co-authored-by: Anirban Ray <39331844+yarnabrina@users.noreply.github.com>
@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 4, 2023

at least for most of the times (except may be releases?).

This might actually be a case for a CRON job?

@hazrulakmal suggested this earlier in another context (checking the upstream data repos' health), here's the link he posted:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

@yarnabrina
Copy link
Member

This might actually be a case for a CRON job?

Sure, that'd work. It'll also mean more frequent through testing than what I suggested above as once per new release.

But I do have a question, not specifically related to CRON. If we intend to run tests for all estimators, how shall we instruct CI to disregard --only_changed_modules True in setup.cfg for only that?

@yarnabrina yarnabrina mentioned this pull request Aug 5, 2023
Merged
@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 5, 2023

If we intend to run tests for all estimators, how shall we instruct CI to disregard --only_changed_modules True in setup.cfg for only that?

The CRON job uses the flag --only_changed_modules False?

Similar to how we override --matrixdesign True to False.

@fkiraly fkiraly changed the title DRAFT [MNT] testing only estimators from modules that have changed compared to main &yarrnabrina [MNT] testing only estimators from modules that have changed compared to main Aug 5, 2023
@fkiraly fkiraly marked this pull request as ready for review August 5, 2023 17:45
@fkiraly fkiraly changed the title &yarrnabrina [MNT] testing only estimators from modules that have changed compared to main &yarnabrina [MNT] testing only estimators from modules that have changed compared to main Aug 5, 2023
@fkiraly fkiraly requested a review from yarnabrina August 9, 2023 10:27
@yarnabrina yarnabrina mentioned this pull request Aug 9, 2023
Merged
@fkiraly fkiraly merged commit 78aacf4 into main Aug 10, 2023
@fkiraly fkiraly deleted the test-only-changes branch August 10, 2023 06:31
@hazrulakmal hazrulakmal mentioned this pull request Aug 11, 2023
Open
@fkiraly fkiraly mentioned this pull request Aug 13, 2023
Open
@yarnabrina yarnabrina mentioned this pull request Oct 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarnabrina yarnabrina yarnabrina 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
maintenance Continuous integration, unit testing & package distribution module:tests test framework functionality - only framework, excl specific tests
Projects
None yet
Milestone
No milestone
Development

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