Skip to content

Conversion to PythonCall#56

Merged
ablaom merged 10 commits intoJuliaAI:devfrom
tylerjthomas9:pythoncall-scikitlearn
Apr 2, 2023
Merged

Conversion to PythonCall#56
ablaom merged 10 commits intoJuliaAI:devfrom
tylerjthomas9:pythoncall-scikitlearn

Conversation

@tylerjthomas9
Copy link
Copy Markdown
Collaborator

@tylerjthomas9 tylerjthomas9 commented Feb 28, 2023

Here is my initial attempt at dropping ScikitLearn.jl, and just including the base PythonCall.jl wrapper. Let me know what you think. I think that it would be great to methods for preparing the data, similar to what we did in CatBoost.jl, but I haven't implemented them yet.

@tylerjthomas9 tylerjthomas9 added the enhancement New feature or request label Feb 28, 2023
Comment thread .github/workflows/ci.yml
- uses: julia-actions/julia-runtest@v1
env:
LD_LIBRARY_PATH: /home/runner/.julia/conda/3/x86_64/lib
- uses: julia-actions/julia-processcoverage@v1
Copy link
Copy Markdown
Member

@ablaom ablaom Mar 1, 2023

Choose a reason for hiding this comment

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

Are we sure this is not still needed? It would seem to me that this issue is orthogonal to the PythonCall/PyCall choice.

@OkonSamuel What say you?

Copy link
Copy Markdown
Collaborator Author

@tylerjthomas9 tylerjthomas9 Mar 1, 2023

Choose a reason for hiding this comment

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

I think that this hack was due to outdated libstdcxx libraries in julia. PythonCall.jl handles this now, and restricts python package versions to only those that are compatible with the current julia session.

edit: this may be needed still for scikit-learn v1.2 to install on older Julia versions. Trying to figure it out.

Comment thread src/ScikitLearnAPI.jl Outdated
Comment thread src/ScikitLearnAPI.jl Outdated
Comment thread test/generic_api_tests.jl
Comment thread test/models/clustering.jl
Comment thread src/models/clustering.jl Outdated
Comment thread src/models/clustering.jl
Copy link
Copy Markdown
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

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

Fantastic work. I can't believe how quickly you pulled this together.

I note that CI is failing on 1.6 Ubuntu. My guess is that a different version of the python libraries are being installed, possible related to @OkonSamuel's libstcxx hack that has been removed??

Is there a way to restrict sk-learn version to [1.2, 1.3)?

Comment thread .github/workflows/ci.yml
@tylerjthomas9
Copy link
Copy Markdown
Collaborator Author

Fantastic work. I can't believe how quickly you pulled this together.

I note that CI is failing on 1.6 Ubuntu. My guess is that a different version of the python libraries are being installed, possible related to @OkonSamuel's libstcxx hack that has been removed??

Is there a way to restrict sk-learn version to [1.2, 1.3)?

On Julia 1.6, scikit-learn v1.1.1 is being installed due to libstdcxx restrictions. I can limit the CondaPkg.toml bounds for scikit-learn, but this would prevent v1.6 from being able to use the package. I will investigate and see if we can just patch the scikit-learn v1.1.1 issue.

@ablaom
Copy link
Copy Markdown
Member

ablaom commented Mar 2, 2023

If you haven't done so already, perhaps you should rebase off the new dev, now that @OkonSamuel's PR is merged?

@OkonSamuel
Copy link
Copy Markdown
Member

Fantastic work. I can't believe how quickly you pulled this together.
I note that CI is failing on 1.6 Ubuntu. My guess is that a different version of the python libraries are being installed, possible related to @OkonSamuel's libstcxx hack that has been removed??
Is there a way to restrict sk-learn version to [1.2, 1.3)?

On Julia 1.6, scikit-learn v1.1.1 is being installed due to libstdcxx restrictions. I can limit the CondaPkg.toml bounds for scikit-learn, but this would prevent v1.6 from being able to use the package. I will investigate and see if we can just patch the scikit-learn v1.1.1 issue.

That's what my hack was meant to address for the CI.
We could just add a readme similar to, https://github.com/cstjean/ScikitLearn.jl#known-issue

@tylerjthomas9
Copy link
Copy Markdown
Collaborator Author

If you haven't done so already, perhaps you should rebase off the new dev, now that @OkonSamuel's PR is merged?

I think that PR has been merged into my branch. I see the changes he made. I will have time after Tuesday of this week to do any remaining tasks to get this PR ready.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 9, 2023

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.07%. Comparing base (89227e3) to head (3840399).
Report is 48 commits behind head on dev.

Files with missing lines Patch % Lines
src/ScikitLearnAPI.jl 80.00% 2 Missing ⚠️
src/macros.jl 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #56      +/-   ##
==========================================
+ Coverage   87.43%   95.07%   +7.63%     
==========================================
  Files          12       13       +1     
  Lines         207      264      +57     
==========================================
+ Hits          181      251      +70     
+ Misses         26       13      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tylerjthomas9
Copy link
Copy Markdown
Collaborator Author

tylerjthomas9 commented Mar 9, 2023

I added a hack to get the CI working on Ubuntu + Julia 1.6. The hack just replaces julia's libstdcxx with the action runner's version, which is more up to date. I am not sure what is different about the github actions version of julia, but it has a older libstdcxx version than julia 1.6 when I install it on my personal computer. For me, the test suite passes with 0 issues on local julia 1.6 installs.

Let me know what you think.

@ablaom
Copy link
Copy Markdown
Member

ablaom commented Mar 9, 2023

@tylerjthomas9 I can't see anything left to do here but I'd like @OkonSamuel to comment on your alternative hack.

Then we should coordinate this merge with the OutlierDetectionPython.jl PR. I'll open a tracking issue shortly.

Comment thread .github/workflows/ci.yml
Comment on lines +44 to +49
libs = filter(x -> ! occursin("32", x), getindex.(split.(readlines(pipeline(`ldconfig -p`, `grep libstdc`)), r"\s*=>\s*"), 2))
source_dir = dirname(libs[end])
julia_lib_dir = joinpath(dirname(Sys.BINDIR), "lib", "julia")
julia_lib_file = get(filter(endswith(r"libstdc\+\+.so\.\d+\.\d+\.\d+"), readdir(julia_lib_dir, join = true)), 1, nothing)
julia_lib_version = match(r"so(\.\d+)\.", julia_lib_file).captures[1]
source_lib = get(filter(endswith(r"libstdc\+\+.so\.\d+\.\d+\.\d+"), readdir(source_dir, join = true)), 1, nothing)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All these is dependent on libs not being empty. Which I think makes sense for Ubuntu builds since there is always a libstdc shared library. Right?

@OkonSamuel
Copy link
Copy Markdown
Member

@tylerjthomas9 I can't see anything left to do here but I'd like @OkonSamuel to comment on your alternative hack.

Then we should coordinate this merge with the OutlierDetectionPython.jl PR. I'll open a tracking issue shortly.

It's the same thing I did. i.e replacing Julia's libstdxx with the up to date system's libstdxx (In my case I replaced it with the libstdxx from the Conda package). Can we document this Hack in the README?

I added a hack to get the CI working on Ubuntu + Julia 1.6. The hack just replaces julia's libstdcxx with the action runner's version, which is more up to date. I am not sure what is different about the github actions version of julia, but it has a older libstdcxx version than julia 1.6 when I install it on my personal computer. For me, the test suite passes with 0 issues on local julia 1.6 installs.

Let me know what you think.

Are you running Ubuntu?

@tylerjthomas9
Copy link
Copy Markdown
Collaborator Author

Added a quick note in the documentation for fixing the libstdcxx issues on Linux. Let me know if I should change anything.

Are you running Ubuntu?

I am. I also tried github codespaces, and had 0 issues with Julia 1.6.7 on Linux.

Copy link
Copy Markdown
Member

@OkonSamuel OkonSamuel left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@OkonSamuel
Copy link
Copy Markdown
Member

Added a quick note in the documentation for fixing the libstdcxx issues on Linux. Let me know if I should change anything.

Are you running Ubuntu?

I am. I also tried github codespaces, and had 0 issues with Julia 1.6.7 on Linux.

That's strange. It fails when I try it on my ubuntu system. Although, I haven't ran any update on my ubuntu for a while now.

@ablaom ablaom merged commit 9b355fc into JuliaAI:dev Apr 2, 2023
@ablaom ablaom mentioned this pull request Apr 2, 2023
@ablaom ablaom changed the title Initial conversion to PythonCall Conversion to PythonCall Apr 2, 2023
@tylerjthomas9 tylerjthomas9 deleted the pythoncall-scikitlearn branch January 6, 2025 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants