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] Dataset downloader testing workflow #5437

Merged
merged 3 commits into from Nov 3, 2023

Conversation

yarnabrina
Copy link
Collaborator

@yarnabrina yarnabrina commented Oct 16, 2023

Replaces #5004
Depends on #5304

@yarnabrina yarnabrina marked this pull request as ready for review October 16, 2023 08:59
@fkiraly fkiraly changed the title [MNT] Dataset workflow [MNT] Dataset downloader testing workflow Oct 22, 2023
@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution module:tests test framework functionality - only framework, excl specific tests enhancement Adding new functionality labels Nov 3, 2023
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.

Nice!

Main problem: this does not remove the download items from the existing tests, so it duplicates, instead of replacing.

Non-blocking: names of the CI steps should be clearer. Writing "step" everywhere is also redundant and makes things harder to read.

@yarnabrina
Copy link
Collaborator Author

@fkiraly Ignoring non-blocking shorter names for the moment, unless you have some ready suggestions. Can be done as part of the new issue you created.

I don't understand the duplicating. New CI do not test datasets at all. It only tests per extra, and even in the new PR where I added test of everything else using all_extras, I intentionally skipped datasets so that this PR is the only one dealing with datasets folder.

Can you please clarify what you mean?

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 3, 2023

Can you please clarify what you mean?

Of course - afaik the dataset downloads are still run as part of the workflows named "install and test", in workflows/test.yml.

So, in a situation where the downloads start failling, they still would paint the test.yml workflows red.

@yarnabrina
Copy link
Collaborator Author

I was focusing on the new CI and didn't want to modify the old CI.

I've removed it from old CI now as per review suggestion. Requesting review.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 3, 2023

thanks! I rememer when I did this in in #5004, it somehow turned off the matrix testing and differential testing logic, and I never figured out why.

@yarnabrina
Copy link
Collaborator Author

Based on job times and number of tests, at least that problem seems to be solved.

(I can't explain why it happened in #5004 though.)

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 3, 2023

Yes, indeed.

@fkiraly fkiraly merged commit 178d9e2 into sktime:main Nov 3, 2023
64 checks passed
@yarnabrina yarnabrina deleted the dataset-workflow branch November 3, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality maintenance Continuous integration, unit testing & package distribution module:tests test framework functionality - only framework, excl specific tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants