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

[enh] Conda recipe update and CI OS version pinning #1992

Merged
merged 16 commits into from
Oct 19, 2024

Conversation

Alexsandruss
Copy link
Contributor

@Alexsandruss Alexsandruss commented Aug 7, 2024

Description

Scikit-learn-intelex conda recipe update.

Changes:

  • Update content of recipe, building and testing scripts to sync with conda-forge feedstock
  • Add recipe building on Linux and Windows to CI
  • Pin CI images versions
  • Add hint for CMake to get python executable from $PYTHON if set

Conda-forge checks: conda-forge/scikit-learn-intelex-feedstock#38


Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • The unit tests pass successfully.
  • I have run it locally and tested the changes extensively.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least summary table with measured data, if performance change is expected.
  • I have provided justification why performance has changed or why changes are not expected.

@Alexsandruss
Copy link
Contributor Author

/intelci: run

@Alexsandruss Alexsandruss force-pushed the dev/conda-build-update branch 3 times, most recently from 631bcc8 to fc31cf2 Compare August 13, 2024 10:20
@Alexsandruss
Copy link
Contributor Author

/intelci: run

1 similar comment
@Alexsandruss
Copy link
Contributor Author

/intelci: run

@Alexsandruss Alexsandruss force-pushed the dev/conda-build-update branch from cde348f to cc66393 Compare August 14, 2024 07:06
@Alexsandruss
Copy link
Contributor Author

/intelci: run

@Alexsandruss Alexsandruss marked this pull request as ready for review August 15, 2024 14:19
@Alexsandruss Alexsandruss force-pushed the dev/conda-build-update branch from 5a920a3 to 48a74af Compare August 26, 2024 21:26
@Alexsandruss
Copy link
Contributor Author

/iintelci: run

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Overall pretty useful addition. I would beef up the description, separate out some of the other changes, and possibly drop sklearn 1.1 or 1.2 from the standard test matrix. The reason being is that by adding these two recipes, we are starting to really push our luck on azure pipeline runners. I'll let others weigh in on this @samir-nasibli @ethanglaser .

scripts/build_backend.py Outdated Show resolved Hide resolved
conda-recipe/run_test.sh Outdated Show resolved Hide resolved
conda-recipe/meta.yaml Outdated Show resolved Hide resolved
conda-recipe/meta.yaml Show resolved Hide resolved
conda-recipe/conda_build_config.yaml Show resolved Hide resolved
.ci/pipeline/nightly.yml Show resolved Hide resolved
.ci/pipeline/build-and-test-win.yml Show resolved Hide resolved
@icfaust icfaust changed the title Conda recipe update [enh] Conda recipe update and CI OS version pinning Aug 28, 2024
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Looks to be some sort of windows python conda issue. Your previous responses were sufficient for me. I found one issue with a re-enablement that may require running private CI, maybe some clarity could explain it away as an alternate solution. If we solve these small issues, I think it would be ready.

conda-recipe/run_test.bat Show resolved Hide resolved
@icfaust
Copy link
Contributor

icfaust commented Aug 28, 2024

/intelci: run

@samir-nasibli
Copy link
Contributor

@Alexsandruss please rebase your branch.

@Alexsandruss
Copy link
Contributor Author

/intelci: run

1 similar comment
@Alexsandruss
Copy link
Contributor Author

/intelci: run

@Alexsandruss
Copy link
Contributor Author

/intelci: run

1 similar comment
@Alexsandruss
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Just some last questions from me. Thanks for the updates based off the previous comments.

.ci/pipeline/ci.yml Outdated Show resolved Hide resolved
.ci/pipeline/ci.yml Show resolved Hide resolved
@david-cortes-intel
Copy link
Contributor

david-cortes-intel commented Oct 16, 2024

Question: are the commands in meta.yaml for executing tests actually needed if it will end up calling the run_test.sh/bat scripts in the end? E.g. do they execute somewhere else too?

.ci/pipeline/ci.yml Outdated Show resolved Hide resolved
.ci/pipeline/ci.yml Outdated Show resolved Hide resolved
.ci/scripts/gen_release_jobs.py Show resolved Hide resolved
conda-recipe/meta.yaml Outdated Show resolved Hide resolved
conda-recipe/meta.yaml Show resolved Hide resolved
conda-recipe/run_test.sh Show resolved Hide resolved
conda-recipe/run_test.bat Outdated Show resolved Hide resolved
conda-recipe/run_test.bat Outdated Show resolved Hide resolved
conda-recipe/run_test.bat Outdated Show resolved Hide resolved
INSTALL.md Outdated

* any `conda` distribution (`miniforge` is recommended)
* `conda-build` and `conda-verify` installed in a conda environment
* (Windows only) Microsoft Visual Studio* 2022
Copy link
Contributor

Choose a reason for hiding this comment

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

can we point to general VS/compiler requirements so we would have single place to track/update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really in case of conda recipe, vs2022 is default for this repo and included in CI Windows image, vs2019 - for conda-forge feedstock, and build config change is required to set another version and there is no guarantee it will work as expected (but it is unlikely someone will use version other than 2019 or 2022).

I will add mention how to change compiler for conda-build on Win.

conda-recipe/run_test.sh Outdated Show resolved Hide resolved
conda-recipe/run_test.sh Show resolved Hide resolved
@Alexsandruss
Copy link
Contributor Author

/intelci: run

@Alexsandruss Alexsandruss merged commit f1655af into uxlfoundation:main Oct 19, 2024
24 of 26 checks passed
@icfaust
Copy link
Contributor

icfaust commented Oct 21, 2024

@Alexsandruss This was merged with an error in the WindowsCondaEnv Python3.12_Sklearn1.5 step

@david-cortes-intel
Copy link
Contributor

@Alexsandruss This was merged with an error in the WindowsCondaEnv Python3.12_Sklearn1.5 step

The error appears to be sporadic though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants