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

Resolve scikit-learn conflict #177

Merged
merged 1 commit into from
Sep 2, 2022
Merged

Conversation

tsj5
Copy link
Collaborator

@tsj5 tsj5 commented Aug 20, 2022

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

PR Checklist

  • This PR has a corresponding issue OR is linked to an SDI.
  • I have followed CliMA's codebase contribution and style guidelines OR N/A.
  • I have followed CliMA's documentation policy.
  • I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
  • I linted my code on my local machine prior to submission: N/A.
  • Unit tests: N/A.
  • Code used in an integration test.
  • All tests ran successfully on my local machine.
  • All classes, modules, and function contain docstrings: N/A.
  • Documentation has been added/updated: N/A.

@tsj5 tsj5 marked this pull request as draft August 20, 2022 03:27
@tsj5
Copy link
Collaborator Author

tsj5 commented Aug 28, 2022

bors try

bors bot added a commit that referenced this pull request Aug 28, 2022
@bors
Copy link
Contributor

bors bot commented Aug 28, 2022

@tsj5 tsj5 changed the title [WIP] Attempt to resolve scikit-learn conflict Resolve scikit-learn conflict Aug 31, 2022
@tsj5 tsj5 self-assigned this Aug 31, 2022
@tsj5 tsj5 linked an issue Aug 31, 2022 that may be closed by this pull request
Pin sklearn to known-good version

Need to pin scipy version as well
@tsj5 tsj5 force-pushed the tsj/zs-manifest-test-1 branch from 679d508 to 934eb38 Compare August 31, 2022 18:45
@tsj5 tsj5 requested a review from odunbar August 31, 2022 18:45
@tsj5 tsj5 marked this pull request as ready for review August 31, 2022 18:45
@tsj5
Copy link
Collaborator Author

tsj5 commented Aug 31, 2022

Merging at request of @odunbar

@tsj5
Copy link
Collaborator Author

tsj5 commented Aug 31, 2022

bors r+

bors bot added a commit that referenced this pull request 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 <pkuszy@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 31, 2022

Timed out.

@tsj5
Copy link
Collaborator Author

tsj5 commented Aug 31, 2022

bors r+

bors bot added a commit that referenced this pull request 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 <pkuszy@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 31, 2022

Timed out.

@odunbar
Copy link
Collaborator

odunbar commented Sep 1, 2022

I think Caltech HPC was down for maintenance yesterday which perhaps is why bors (buildkite in particular) timed out
rerunning to check

@odunbar
Copy link
Collaborator

odunbar commented Sep 1, 2022

bors r+

bors bot added a commit that referenced this pull request 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 <pkuszy@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 1, 2022

Timed out.

@odunbar
Copy link
Collaborator

odunbar commented Sep 2, 2022

bors try

bors bot added a commit that referenced this pull request Sep 2, 2022
@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

try

Build failed:

@odunbar
Copy link
Collaborator

odunbar commented Sep 2, 2022

bors try

bors bot added a commit that referenced this pull request Sep 2, 2022
@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

@tsj5
Copy link
Collaborator Author

tsj5 commented Sep 2, 2022

Test on linux failed due to known issue #169. Merging this PR under the expectation that this intermittent failure will be fixed in a future PR addressing that issue.

try

Build failed:

@tsj5
Copy link
Collaborator Author

tsj5 commented Sep 2, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

@bors bors bot merged commit 5eb0701 into CliMA:master Sep 2, 2022
@tsj5 tsj5 mentioned this pull request 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 this pull request may close these issues.

SciKitLearn python backend update breaks GH testing Some packages in Manifest are outdated
4 participants