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

[ENH] removed py37.dockerfile and update doc entry for CI #5356

Merged
merged 4 commits into from Oct 8, 2023
Merged

[ENH] removed py37.dockerfile and update doc entry for CI #5356

merged 4 commits into from Oct 8, 2023

Conversation

kurayami07734
Copy link
Contributor

@kurayami07734 kurayami07734 commented Oct 4, 2023 •
edited

Changes

  • Removed dockerized test for python 3.7 (as it has reached EOL)
  • Updated corresponding doc entry for dockerized test

Concerns

While trying to update the other dockerfiles, I came across a bug (See also #5352), such that tests are unable to run.

@yarnabrina
Copy link
Collaborator

yarnabrina commented Oct 4, 2023 •
edited

Thanks for your contribution.

I'll suggest to update the base images, e.g. from 3.8.16-bullseye to 3.8-slim-bullseye. This will use latest patch releases, and will also decrease the image size. Merging the two RUN commands can help in that as well.

Note: slim may require installation of few system dependencies separately with apt-get or something, as by default a lot of things are installed in the non-slim image.

Also, I think we now recommend all_extras_pandas2 instead of all_extras.

@fkiraly if you think it's better not make these changes, or out of scope for this PR, let me know.

@kurayami07734
Copy link
Contributor Author

Please consider adding the hacktoberfest label to this issue

@achieveordie
Copy link
Collaborator

If we are changing our base image, we might as well choose bookworm which is the current stable version of Debian. Info about bookworm. We can choose to use the more efficient version as @yarnabrina suggested but I think that is beyond the scope of this PR.

I've created a new issue #5359 where we can discuss the appropriate choice for our base image.

@achieveordie achieveordie mentioned this pull request Oct 4, 2023
Closed
@yarnabrina
Copy link
Collaborator

Agreed on bookworm. We can use just 3.8-slim etc. then I suppose.

@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Oct 4, 2023
fkiraly
fkiraly previously approved these changes Oct 4, 2023
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.

@kurayami07734, this does not close #5309 since only part is carried out - having said that, it seems fine to merge, a 3.7 has reached EOL

@kurayami07734
Copy link
Contributor Author

Why are the workflows not running?

@achieveordie
Copy link
Collaborator

PRs from first time contributors need to be specifically approved before they can run. I've started the runners, this PR should pass without any problems since we only have documentation changes here.

Thanks for the contribution!

@kurayami07734
Copy link
Contributor Author

It would be really nice, if you could add the hacktoberfest labels

achieveordie
achieveordie previously approved these changes Oct 6, 2023
View reviewed changes
@fkiraly
Copy link
Collaborator

fkiraly commented Oct 6, 2023 •
edited

Please consider adding the hacktoberfest label to this issue

The repo has the tag so it would count, no need to extra tag PRs.

Just read @yarnabrina's request, would you mind also addressing that i a separate PR, while we are at it, @kurayami07734?

@kurayami07734
Copy link
Contributor Author

kurayami07734 commented Oct 6, 2023 •
edited

The repo has the tag so it would count, no need to extra tag PRs.

screenshot-2023-10-06_23:39:15
I had asked because of the above

Just read @yarnabrina's request, would you mind also addressing that i a separate PR, while we are at it, @kurayami07734?

Sure, I have no problem with that
But how would I run the tests (See #5352)

@github-actions github-actions bot dismissed stale reviews from achieveordie and fkiraly via c196d6d October 6, 2023 18:20
@fkiraly
Copy link
Collaborator

fkiraly commented Oct 6, 2023

I had asked because of the above

I just added it so you may have to reload

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 6, 2023 •
edited

But how would I run the tests (See #5352)

You would have to diagnose and repair the issue with the missing branch first.

@fkiraly fkiraly mentioned this pull request Oct 7, 2023
Closed
@kurayami07734
Copy link
Contributor Author

kurayami07734 commented Oct 8, 2023 •
edited

How has this conflict emerged?
Also please merge this PR

fkiraly
fkiraly approved these changes Oct 8, 2023
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment •
edited

Choose a reason for hiding this comment

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

Sure, we can merge this.

The conflict appeared since you didn't update from main regularly - it is recommended to update from main regularly while you develop. In this specific case, the automatic contributor list generation conflicted with the list from this PR and the list from main.

@fkiraly fkiraly merged commit d83c6aa into sktime:main Oct 8, 2023
23 of 24 checks passed
@yarnabrina yarnabrina mentioned this pull request Oct 10, 2023
Closed
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into validate-extras-workflow
c31a6fe 
* 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 added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into split-ci
2132e16 
* 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 added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-base
425ede2 
* origin/split-ci:
  control pytest configuration
  [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 added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-fo…
8eb36b5 
…recasting

* origin/split-ci:
  control pytest configuration
  [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)
@fkiraly fkiraly mentioned this pull request Jan 25, 2024
Closed
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 achieveordie left review comments

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

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

Assignees
No one assigned
Labels
maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kurayami07734 @yarnabrina @achieveordie @fkiraly