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

DOC small documentation changes in the advanced installation guide #26394

Merged

Conversation

marenwestermann
Copy link
Member

Reference Issues/PRs

follow up from PR #26334
towards #25985

What does this implement/fix? Explain your changes.

  • makes pip install commands uniform in the contributing to scikit-learn documentation
  • adds short explanation for why the --no-use-pep517 flag is needed when building from source

Any other comments?

ping @Micky774

@marenwestermann
Copy link
Member Author

@Micky774 after reading the PEP 517 documentation I still don't have a good understanding of what it does. So I just added a short explanation for why it is needed when building from source. Let me know if it's correct or if you would like to see any other changes.

@Micky774
Copy link
Contributor

@adrinjalali @marenwestermann do either of you have a link to a discussion regarding the necessity of --no-use-pep517 with --no-build-isolation? I think it would be good to link it in the note addendum in this PR.

@adrinjalali
Copy link
Member

I'm not sure, but it seems pep517 is the pyproject.toml style kinda thing, for which editable mode is not allowed?

pypa/pip#6442

I really don't know why, but it seems they're related. There are a lot of related issues on the pip repo, and they don't give me a good explanation. So I'm not sure how we can explain it here lol

@glemaitre
Copy link
Member

Are we sure that the PEP517 flag is necessary?

I created an environment from scratch and tried the instruction and it seems to work.
I might have a recent pip and I don't know if the implementation of the PEP660 solved the problem.

Here is the end of the output of my installation:

Editable install will be performed using a meta path finder.

  Options like `package-data`, `include/exclude-package-data` or
  `packages.find.exclude/include` may have no effect.

  adding '__editable___scikit_learn_1_4_dev0_finder.py'
  adding '__editable__.scikit_learn-1.4.dev0.pth'
  creating '/tmp/pip-wheel-5fmi1pu2/.tmp-pxhfybdj/scikit_learn-1.4.dev0-0.editable-cp39-cp39-linux_x86_64.whl' and adding '/tmp/tmpf4n112bkscikit_learn-1.4.dev0-0.editable-cp39-cp39-linux_x86_64.whl' to it
  adding 'scikit_learn-1.4.dev0.dist-info/COPYING'
  adding 'scikit_learn-1.4.dev0.dist-info/METADATA'
  adding 'scikit_learn-1.4.dev0.dist-info/WHEEL'
  adding 'scikit_learn-1.4.dev0.dist-info/top_level.txt'
  adding 'scikit_learn-1.4.dev0.dist-info/RECORD'
  /home/glemaitre/miniconda3/envs/sklearn-env/lib/python3.9/site-packages/setuptools/command/editable_wheel.py:348: InformationOnly: Editable installation.
  !!

          ********************************************************************************
          Please be careful with folders in your working directory with the same
          name as your package as they may take precedence during imports.
          ********************************************************************************

  !!
    wheel_obj.write_files(unpacked)
  Building editable for scikit-learn (pyproject.toml) ... done
  Created wheel for scikit-learn: filename=scikit_learn-1.4.dev0-0.editable-cp39-cp39-linux_x86_64.whl size=6794 sha256=ce84def516ebb2249d15425ed2686a4d2725cbb503bc7a131698042c908d7bf6
  Stored in directory: /tmp/pip-ephem-wheel-cache-g_whz2ov/wheels/ee/6e/16/0f21158be67077cef3936c48c77c622576e2a4aca15b696e07
Successfully built scikit-learn
Installing collected packages: threadpoolctl, joblib, scikit-learn
Successfully installed joblib-1.2.0 scikit-learn-1.4.dev0 threadpoolctl-3.1.0

@Micky774
Copy link
Contributor

Are we sure that the PEP517 flag is necessary?

That's what I'm doubting as well. I believe at some point in discussion @rgommers mentioned that it should not be necessary nowadays (I don't recall the specifics). I'm not sure it is actually required at this point, or if it is required only in specific cases?

@glemaitre
Copy link
Member

if it is required only in specific cases

With old pip version certainly.

@adrinjalali
Copy link
Member

Yes, we definitely need the pep517 flag. Without the flag, every build will take as long as a fresh build, instead of only building the changed files. You can replicated that by trying it twice.

@glemaitre
Copy link
Member

You can replicated that by trying it twice.

Oh I see. Since I am always using make in whenever rebuilding, I never saw this aspect.

Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

I'm okay with merging the PR as-is. We can always include additional notes later if we have some phrasing we'd really like to communicate to users.

@Micky774 Micky774 merged commit 96e13f1 into scikit-learn:main Jun 27, 2023
29 checks passed
@Micky774
Copy link
Contributor

Thank you very much @marenwestermann 😄!

@marenwestermann marenwestermann deleted the doc-advanced-installation branch June 27, 2023 17:55
@marenwestermann
Copy link
Member Author

Thanks a lot everyone for the discussion! :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants