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

SciKitLearn python backend update breaks GH testing #176

Closed
odunbar opened this issue Aug 19, 2022 · 2 comments · Fixed by #177
Closed

SciKitLearn python backend update breaks GH testing #176

odunbar opened this issue Aug 19, 2022 · 2 comments · Fixed by #177

Comments

@odunbar
Copy link
Collaborator

odunbar commented Aug 19, 2022

Old working version saw conda install
scikit-learn 1.1.1 and scipy 1.8.1
Now conda installs
scikit-learn 1.1.2 and scipy 1.9.0
I suspect the syntax for pyimport has changed

@tsj5
Copy link
Collaborator

tsj5 commented Aug 20, 2022

This is puzzling -- the reason for the broken test appears to be that julia gets linked against one version of libstdc++, while the scikit-learn stuff wants a different version, as described here. The workaround (here) is to modify LD_PRELOAD; apparently there's no less brittle fix, since the issue is with the third-party scikit-learn dependency.

Aspects I'm unclear on:

  • Is it necessary to use the LD_PRELOAD hack for any user of the scikit-learn backend on linux (seems likely)? It would be simple enough to modify LD_PRELOAD as part of the setup in Tests.yml, but this wouldn't be feasible to ask an end user to do every time they ran CES.
  • What specific change to Manifest.toml brought in the incompatible scikit-learn? Conda.jl appears to pass through version specifications, so we could say julia --project -e 'using Conda; Conda.add("scikit-learn=1.1.1")', but this means we'd be stuck maintaining a compatibility matrix between julia versions and scikit-learn versions.

@tsj5 tsj5 mentioned this issue Aug 20, 2022
10 tasks
@tsj5
Copy link
Collaborator

tsj5 commented Aug 20, 2022

Pinning the versions of scikit-learn and scipy appears to get the linux test to pass (PR #177). Should we proceed along these lines?

@tsj5 tsj5 linked a pull request Aug 31, 2022 that will close this issue
10 tasks
bors bot added a commit that referenced this issue Aug 31, 2022
177: Resolve scikit-learn conflict r=tsj5 a=tsj5

# PULL REQUEST

## Purpose and Content
This PR fixes the problems encountered by `@szy21` in #175. The purpose of that PR was to update the Manifest for the examples (Project/Manifest for CES itself is unchanged). This PR uses `@szy21` 's updates and in addition pins the versions of scipy and scikit-learn installed through Conda.jl to avoid an incompatibility in libstdc++ (documented in #176).

## Benefits and Risks

**Benefits**: Resolves #176. `@szy21's` original PR resolves #174.

**Risks**: Introduces maintenance requirement of having to remember we've pinned the versions of some dependencies going forward; since these are python dependencies this information isn't in Project.toml, so we might forget about this in the future when we want/need to update scikit-learn. This is a very minor risk, as the functionality from sk-learn that we currently use is mature.

## Linked Issues
- Closes #174, #176 

## PR Checklist
- [X] This PR has a corresponding issue OR is linked to an SDI.
- [X] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [X] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [X] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [X] I linted my code on my local machine prior to submission: N/A.
- [X] Unit tests: N/A.
- [X] Code used in an integration test.
- [X] All tests ran successfully on my local machine.
- [X] All classes, modules, and function contain docstrings: N/A.
- [X] Documentation has been added/updated: N/A.


Co-authored-by: szy21 <[email protected]>
bors bot added a commit that referenced this issue Aug 31, 2022
177: Resolve scikit-learn conflict r=tsj5 a=tsj5

# PULL REQUEST

## Purpose and Content
This PR fixes the problems encountered by `@szy21` in #175. The purpose of that PR was to update the Manifest for the examples (Project/Manifest for CES itself is unchanged). This PR uses `@szy21` 's updates and in addition pins the versions of scipy and scikit-learn installed through Conda.jl to avoid an incompatibility in libstdc++ (documented in #176).

## Benefits and Risks

**Benefits**: Resolves #176. `@szy21's` original PR resolves #174.

**Risks**: Introduces maintenance requirement of having to remember we've pinned the versions of some dependencies going forward; since these are python dependencies this information isn't in Project.toml, so we might forget about this in the future when we want/need to update scikit-learn. This is a very minor risk, as the functionality from sk-learn that we currently use is mature.

## Linked Issues
- Closes #174, #176 

## PR Checklist
- [X] This PR has a corresponding issue OR is linked to an SDI.
- [X] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [X] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [X] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [X] I linted my code on my local machine prior to submission: N/A.
- [X] Unit tests: N/A.
- [X] Code used in an integration test.
- [X] All tests ran successfully on my local machine.
- [X] All classes, modules, and function contain docstrings: N/A.
- [X] Documentation has been added/updated: N/A.


Co-authored-by: szy21 <[email protected]>
bors bot added a commit that referenced this issue Sep 1, 2022
177: Resolve scikit-learn conflict r=odunbar a=tsj5

# PULL REQUEST

## Purpose and Content
This PR fixes the problems encountered by `@szy21` in #175. The purpose of that PR was to update the Manifest for the examples (Project/Manifest for CES itself is unchanged). This PR uses `@szy21` 's updates and in addition pins the versions of scipy and scikit-learn installed through Conda.jl to avoid an incompatibility in libstdc++ (documented in #176).

## Benefits and Risks

**Benefits**: Resolves #176. `@szy21's` original PR resolves #174.

**Risks**: Introduces maintenance requirement of having to remember we've pinned the versions of some dependencies going forward; since these are python dependencies this information isn't in Project.toml, so we might forget about this in the future when we want/need to update scikit-learn. This is a very minor risk, as the functionality from sk-learn that we currently use is mature.

## Linked Issues
- Closes #174, #176 

## PR Checklist
- [X] This PR has a corresponding issue OR is linked to an SDI.
- [X] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [X] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [X] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [X] I linted my code on my local machine prior to submission: N/A.
- [X] Unit tests: N/A.
- [X] Code used in an integration test.
- [X] All tests ran successfully on my local machine.
- [X] All classes, modules, and function contain docstrings: N/A.
- [X] Documentation has been added/updated: N/A.


Co-authored-by: szy21 <[email protected]>
@bors bors bot closed this as completed in 5eb0701 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants