[MNT] remove redundant install-only CI jobs

[fkiraly]

This PR removes redundant install-only CI jobs, see discussion #5706 (comment)

Why redundant: as far as I understand, we install the same environments anyway in later jobs that include pytest runs.

So, if the install would fail, we would equally notice, and the later test jobs would simply not proceed beyond the install step.

[fkiraly]

FYI @yarnabrina

[yarnabrina]

As I just commented at #5706 (comment), I think this is risky as it's not going to check anything for PR's created by Dependabo, or any other PR which just modifies pyproject.toml and nothing else. It's not going to run unit tests either way, but at least documented extra will be a valid specification.

My personal preference will be to not merge this, but if it's significantly more inconvenient for you and other reviewers because of high number of jobs than it'd be hypothetically safer for a potential bad dependency upgrade PR, please ignore this comment.

[fkiraly]

@yarnabrina, I am getting confused, can you please phrase your statement as

• after this PR is merged
• in tests for a PR where pyproject.toml is changed but no other file,
• failure condition X is not covered anymore
• because Y (explain why other CI elements do not cove)
• but it would be covered before the PR is merged because Z (name CI elements)

[yarnabrina]

I'm unfamiliar with this format, but this is my attempt at it, assuming absence of "old CI".

1. Assument this PR ([MNT] remove redundant install-only CI jobs #5718) is merged.

2. Dependabot creates a PR similar to PR [MNT] [Dependabot](deps-dev): Update arch requirement from <6.3.0,>=5.6 to >=5.6,<6.4.0 #5705 from main branch.

3. No change is detected in any of the sktime components (sub-directories of sktime folder in the repository) -> only code_quality job will run and nothing else (jobs to detect changes will run, but not any test of any kind).

4. If the new PR updated a package version incorrectly, i.e. something that is not working in all supported environments, we will not know from "new CI" at all. It will not only break functionality of that particular estimators, but all the while "extra" itself as there's nothing like a partial install in pip (if something fails during installation, nothing gets installed). An example is PR [MNT] [Dependabot](deps-dev): Update pycatch22 requirement from <0.4.4 to <0.4.5 #5442.

5. Currently, that extra (and all others till [MNT] Identify modified extra(s) in PR's that modify pyproject.toml #5477 is addressed) will be installed everywhere so at least we'll at least know if the "extra" specification is valid and can be used by users workflow without breaking. As of now, we'll not know if the estimatora that need it will support this version or not (till [ENH][MNT] testing version bumps, e.g., dependabot upgrades - how to ensure objets affected by soft dependency changes are tested #5292 is addressed), but at least it won't affect other estimators from same "component".

[fkiraly]

Thanks - could you kindly be explicit: what is the failure condition that is not covered?

Is it "sktime[all_extras] installs", or is it "all tests run with env sktime[all_extras], or sth else?

[yarnabrina]

I don't know what you're asking, I'm very confused.

I'm talking about cases like #5442 which updates a dependency specification (in this example upgrade upper bound of pycatch22) which is not supported in all environments (in this example, windows). This situation will not be tested at all in new CI if you merge this PR.

If this is not merged, validate-extras workflow will identify the environment where the updated specification is unsupported.

[fkiraly]

I don't know what you're asking, I'm very confused.

Very sorry, seems like a terminology, definition, or other commuication issue to me - that is why I was suggesting a clean write-up with defined terms.

Maybe if I start:

"failure condition" -> a logical condition which a test or set of tests covers. "X is the failure condition for test(s) Y" means, if X fails, the the test Y fails. Also: "Y certifies for X". Can also be used in the sense that "Y certifies for a bugfix of X". Example: test_constructor covers interface compliance of __init__ of all BaseObject descendants. It allso certifies for fixes to most knonwn bugs impactign __init__.

My question can be phrased as: what is the exact failure condition that you are talking about, and please explain, why does only validate-extras cover it, among the new CI elements.

[yarnabrina]

Given your above explanation, I think #5718 (comment) answers it.

what is the exact failure condition that you are talking about

Someone (or dependabot) modifies a dependency specification (one of module-specific-dependency-sets or all_extras_pandas2) and the modification is not supported in all 3 operating system * 5 python combinations (at least one incompatible environment).

why does only validate-extras cover it

If the PR does not modify anything other than pyproject.toml, neither test_base nor test_components_with_extras nor test_components_without_extras run any unit tests, as they all depend on change detection logic, which will identify no modification in the tracked files.

So your expectation of indirect installation validation during these workflows won't be fulfilled, which is the basis of this PR as per my understanding.

[fkiraly]

I see.

Understanding that, I think the run condition should be "pyproject.toml has changed", not "always".
Do you know how to change that, @yarnabrina?

[fkiraly]

After discussion in #5706, I understand this is not redundant but triggered only if pyproject.toml is modifiedl

[yarnabrina]

I see.

Understanding that, I think the run condition should be "pyproject.toml has changed", not "always". Do you know how to change that, @yarnabrina?

It's that way already when CI is triggered from a PR, referring the link here for future references.

sktime/.github/workflows/test_code_quality.yml

Line 48 in 3d0efc8

if: ${{ needs.detect.outputs.pyproject == 'true' }}