-
Notifications
You must be signed in to change notification settings - Fork 604
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
Switch to flit #1527
Switch to flit #1527
Conversation
9cbeee8
to
5d34d96
Compare
Current problems:
Not a problem (to my knowledge), but because you mentioned it: conda doesn’t identify flit installed distributions as |
I had run into a problem with
|
I figured it out: pypa/setuptools#2531 |
Due to the first problem (that blocked using flit in all circumstances) being fixed and the last two standing in the way of the @ivirshup what do you think? PS: here’s some progress on standardized --editable mode pypa/pip#8212 |
I'm not comfortable with doing an absolute switch without significant testing. I would want a few frequent contributors (Fidel, Sergei, Goecken?) to try it out, try and break it, and make sure we could get things done. I also still have concerns about the limitations of what flit can distribute, and would like to hear other's thoughts on this. If a |
Ha, got it! Seems like all tools break differently on multiline strings in the metadata. I changed the string back to single ticks to test that metadata problem and then forgot. Now it works and is tested! Also I checked and |
c622322
to
1d871a7
Compare
Huh, very interesting. Why did you choose flit over https://python-poetry.org/ ? |
Poetry is great! But i remember two problems:
Maybe @ivirshup knows more. We talked about it way back when. The things I’m missing from flit are better dynamic version support (currently a bit hacky, but a PR exists) and sth. like |
I've found poetry mostly a pain the last few times I've tried it. Maybe it's gotten better, but the I like that flit is much more limited in scope, and does not try to do everything. I'd also be worried using |
All right, fair points.
I also stumbled upon the editably install issue. This is not an issue that Poetry can solve at the moment as explained in the thread. I do however understand that this is an issue for scanpy (considering the strong anndata dependency etc). Regarding plugins - they are on the roadmap and should appear at some point. Considering that the community is very active whereas the main developer is not anymore this may solve the editable install with some hack as well. I agree with your points and Poetry is not yet the solution that we should currently use, but I think it is the proper solution that we should aim for.
I don't think that anybody is familiar with flit either, but it is slightly less intrusive and does not fundamentally change so many things like Poetry does. However, many things that Poetry does change make a lot of sense and solve other issues that we did not discuss here yet. So yeah, I personally would wait for Poetry to get it's plugin system and for the editable install issue to get a proper PEP. But if you don't feel like waiting Flit might be fun :) Cheers |
Flit has a very tiny surface area. You can learn its full CLI in literally 2 minutes, as it doesn’t include any kind of new concept (like Poetry’s venv management). |
Yeah, I agree. Go for it ;) Saturday, 23 January 2021, 11:48AM +01:00 from Philipp A. notifications@github.com :
…Flit has a very tiny surface area. You can learn its full CLI in literally 2 minutes, as it doesn’t include any kind of new concept (like Poetry’s venv management).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub , or unsubscribe .
|
@Zethson as asked in your PR, can you please add a commit to fix the conflict here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flying-sheep, just gave this another try and ran into problems with the symlink installation. Here's what's happening
# Create environment
conda create -yn flit-symlink-test python=3.8 flit gh
conda activate flit-symlink-test
# Get scanpy and install
git clone https://github.com/theislab/scanpy.git
cd scanpy
gh pr checkout 1527
flit install -s --deps=develop
# Check that install worked
cd ..
python -c "import scanpy; print(scanpy.__version__)"
# 1.7.0rc2.dev12+ga760655a
# Check that code is modified when I change version
cd scanpy
git checkout 1.7.0
cd ..
python -c "import scanpy; print(scanpy.__version__)"
# 1.7.0rc2.dev12+ga760655a
The second attempt to print the scanpy version should print 1.7.0
.
This problem appears to be specific to the symlink install. If I install the package with flit install --pth-file --deps=develop
or pip install -e ".[dev, test]
the correct version is printed.
It does look like scanpy is being symlinked correctly:
$ python -c "import scanpy; print(scanpy.__file__)"
/Users/isaac/miniconda3/envs/flit-symlink-test/lib/python3.8/site-packages/scanpy/__init__.py
$ python -c "import scanpy; from pathlib import Path; print(Path(scanpy.__file__).resolve())"
/Users/isaac/Documents/Issues/scanpy_flit/manual_test/scanpy/scanpy/__init__.py
And, interestingly, the version is reported correctly if we are in the scanpy directory
cd scanpy
python -c "import scanpy; print(scanpy.__version__)"
# 1.7.0
As a side note, I've been using a little bit of scripting to help automate testing this PR. It's in julia
, but I'll include that here incase it's helpful to the PRs progress.
using Base.Iterators
install_dict = Dict([
("flit-pth", `flit install --pth-file --deps=develop`),
("flit-sym", `flit install -s --deps=develop`),
("pip-edit", `pip install -e ".[dev, test]"`),
])
names = collect(keys(install_dict))
setup_url = "https://gist.githubusercontent.com/flying-sheep/9e7776f4cbe967397702e1c581e3a40a/raw/a2707597abf90f14efdb37751bdd2330cc4d1359/setup.py"
install_results = Dict{String, Tuple{String, String}}()
import_results = Dict{String, Tuple{String, String}}()
# settings = collect(product(install_dict, (true, false)))
# names = map(settings) do ((install_type, install_command), w_setup)
# w_setup ? "$(install_type)_setup" : install_type
# end
function create_envs!(names)
procs = map(
name -> run(`conda create -yn $(name) python=3.8 flit`, wait=false),
names
)
while !all(process_exited.(procs))
sleep(0.1)
end
@assert all(success.(procs))
procs
end
function delete_envs!(names)
procs = map(
name -> run(`conda env remove -yn $(name)`, wait=false),
names
)
while !all(process_exited.(procs))
sleep(0.1)
end
@assert all(success.(procs))
end
function setup_directory!(name, w_setup=false)
if isdir(name)
rm(name, recursive=true)
end
mkdir(name)
cd(name) do
run(`git clone https://github.com/theislab/scanpy.git`, wait=true)
cd("scanpy") do
run(`gh pr checkout 1527`)
if w_setup && !isfile("setup.py")
run(`wget $(setup_url)`)
end
end
end
end
function read_permissive(cmd)
stdout = IOBuffer()
stderr = IOBuffer()
proc = run(pipeline(cmd; stdout, stderr), wait=false)
wait(proc) # hmm
String(take!(stdout)), String(take!(stderr))
end
function install!(name, install_command)
cd("$(name)/scanpy") do
read_permissive(`conda run -n $(name) $(install_command)`)
end
end
env_contents(name) = read(`conda env list -n $(name)`, String)
function check_version(name)
read_permissive(`conda run -n $(name) python -c "import scanpy; print(scanpy.__version__)"`)
end
function checkout_ref!(name, ref)
cd("$(name)/scanpy") do
run(`git checkout $(ref)`)
end
end
# setup
# delete_envs!(names) # If already made
create_envs!(names)
# Create directories
setup_directory!.(names)
# Try to install
for (name, cmd) in install_dict
println(name)
out, err = install!(name, cmd)
if !isempty(err)
println(err)
end
print("\n\n")
end
check_version.(names)
checkout_ref!.(names, "1.7.0")
check_version.(names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments (and code review)
For the .azure-pipelines
conflicts, could you apply your changes on top of the master version of that config?
Also, could the "Build and twine check" job build with flit
?
Time to merge? |
@adamgayoso, I'd be interested to hear about what you like about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay all done. flit install -s was getting too messy as some dependency installs scanpy and then things can’t be symlinked …
Can you elaborate on this? This sounds like it could be a more general problem.
When I try this:
$ conda create -yn "test-flit-sym" python=3.8 flit
$ conda activate test-flit-sym
$ flit install -s --deps=develop
$ pip install scvelo
...
Attempting uninstall: scanpy
Found existing installation: scanpy 1.7.0rc2.dev33-gf953fbc8.d20210218
Uninstalling scanpy-1.7.0rc2.dev33-gf953fbc8.d20210218:
Successfully uninstalled scanpy-1.7.0rc2.dev33-gf953fbc8.d20210218
Successfully installed loompy-3.0.6 numpy-groupies-0.9.13 scanpy-1.7.0 ...
This seems like pretty bad behavior for a development environment. We definitely don't want the dev install to be uninstalled when a new package gets downloaded.
Why is this even happening, and is it fixable?
Less importantly, if you then run flit install -s --deps=develop
, the version reported by pip does not get updated.
$ flit install -s --deps=develop
$ pip list | grep scanpy
scanpy 1.7.0
It looks like it actually does get overwritten, but it's non-obvious.
better leave the pip install -r in temporarily
Why not just use pip install -e
here? I'm a bit concerned that pinning pip to an old version could lead to problems, especially while pip is going through a lot of changes.
Quick recap of things remaining (summary for myself)
- Exclude
setup.py
from sdist using the standard way, not via.gitignore
flit
mangles the build version part of wheel filenames, in a way thatpip
just started checking for. We can work around this in CI, and should not affect distributions (as there is no build info there). I believe this will breakpip install git+https:{repo}
package from the repo.pip install -e
still works.flit
symlinked packages seem to be overwritten if a new package is installed which has the symlinked package as a dependency.- There is a fairly large workaround to make the package version available if the dependencies are not installed. Is it possible to use something more standard like versioneer here?
Well, scvelo depends on 1.7 and you have a release candidate of that one installed, so what happened is the only correct behavior: It uninstalled an incompatible version to install a compatible one. If your install’s metadata was outdated and it was in fact a compatible one, then you forgot to refresh the metadata by reinstalling it. That’s annoying but necessary as editable installs are nonstandard and therefore not well integrated into how package metadata works.
Because development installs in general are nonstandard, and Tasks
sounds good!
No, as far as I can see, pip arbitrarily decided to not allow local version specifiers in wheel filenames. AFAIK nothing says there can’t be pluses in there, only that you can’t upload packages with local specifiers in their version to PyPI. Which we don’t do here, so pip should chill. If flit decides to work around that quirk, or pip relaxes, we can unpin pip.
Seee above. Has nothing to do with flit. What made you thing that anyway?
No. Either we hardcode a string constant in the That’s the only disadvantage flit has IMHO, but we discussed that at length in the past and found it to not be a problem as the hack is robust and well documented. |
I'm not sure what you mean by this. Does
I think pinning pip to an old version is worse than using a common, even if non-standard, installation method.
My reading of the PR in |
Ah weird. Pip tries to resolve the dependencies, and for that purpose gets all the installed packages’ metadata, then tries to figure out a configuration of upgrades that makes things work. No idea why it sees “1.7.0rc2” and decides “I’ll update this even when not asked to update”. Maybe raise this issue with pip?
Yes, that’s a reinstall in some development mode. My point was that if a scanpy pre-1.7 version really was installed, maybe pip was correct to update to 1.7 for some reason. However since we’re past 1.7 now, unless you haven’t git-pulled yet, I assume your dev install’s metadata has gone stale. I guess pip wouldn’t uninstall your dev install if you had run
Our setup.py is a compatibility shim solely for fallback use, not something to be relied upon in any part of our process. Usually when something does an arbitrary change making our life harder, our approach is pinning it temporarily until it fixed that or the infrastructure has adapted to its whims, right?
Accepted PEPs are specs, so only pip and flit can be right or wrong (as they implement it). If people decide that what pip does happens to be better than the currently spec-compliant behavior, the spec can be changed accordingly. Until then pip is wrong, so we should pin its version to one that accepts spec-compliant behavior. (we can change our approach if that happens to drag on too long) I see you already commented in pypa/pip#9628, so I guess that’s the better place for following that. |
symlink installs being uninstalled (most important)
Yeah, this is super weird. I think it's also blocking for adopting I see two paths forward here:
Pinning Pip on CI
I still have the concern that pinning pip to an old version could lead to problems, especially while pip is going through a lot of changes. But we can leave this for now. If getting this wheel issue solved drags on for multiple pip versions, we may need to reconsider. PEP stuff
I think that conversation is happening in multiple places, so might be hard to track. Installing from the repoAs it stands: conda create -n scanpyenv python=3.8
https://github.com/theislab/scanpy.git
cd scanpy
pip install . Will error, unless the commit at the tip of master happens to be tagged with a release version. Right now I don't think this is an issue since I wouldn't expect anyone to install from github unless they were setting up a development environment. And if they are setting up a dev environment, they should be using I'm not 100% confident this isn't an issue, and it would be good to get more opinions on this. Version resolution
On how version strings are handled/ generated: I would be more comfortable using a solution that other packages used too. In particular, this looks very brittle to me: for frame in traceback.extract_stack():
if frame.name == 'get_docstring_and_version_via_import':
return True I don't see why |
I believe the issues are related. I think pip is trying to get the version of the wheel by parsing it's name instead. For example, I just ran into this with where scanpy's version was being reported as: |
Damn more merge conflicts. I’ll fix it one last time then merge this, OK? Unfortunately this needs to be a rebase, otherwise I have to resolve everything again when rebasing. |
How has that to do with flit? Will pip just block upgrades things it identifies as “editable installs” from being upgraded while happily upgrading “normally installed” release candidates?
Exactly!
It wasn’t an issue for the year this PR has lingered, and now it’s pypa/pip#9628 and related discussions (which are already converging) I would like to meaningfully contribute to scanpy again instead of having to fix merge conflicts in this and discussing what pip broke this time.
It isn’t, as you agreed on like 8 months ago. Worst that happens is “ImportError: cannot import ‘foo’” when building, which we can fix in 5 minutes. |
5efea82
to
e715cd9
Compare
I would also like to see this merge. I've put a lot of time and effort into reviewing it, so we can get this over and done with. The reason I'm so hard on this is that it's critical to our project (and getting new contributors), and it's a part of the stack I don't understand.
I don't recall this specifically from 8 months ago. Theres a good chance that because I don't have as great of a knowledge about how packaging works, I understood our conversation in a different way. Because this is new, there's definitley going to be bugs. These are bugs with part of the stack that we don't have a lot of expertise in, so I'd like to minimize these before they become blockers.
This is bad, and should not be the default experience for people who want to contribute. This does not give an error, or a warning. Yes there are solutions being proposed upstream, but we don't know how long until they are implemented. Here's what I propose. I think this can be merged basically as is. However, until these issues are resolved: development installation instructions has to have |
thank you, I really appreciate this ❤️
Totally understood. My motivation to use flit is that it’s simpler and therefore better both for first-time contributors (to get started) and experienced people (to debug), whereas CLI, metadata, and code of setuptools/pip is very complex and a nightmare to debug. I know that due to flit being used less, there needs to be someone who understands the packaging ecosystem to fix things when they’re broken instead of cargo-culting one of the million answers around setuptools on StackOverflow.
OK, will do! Can you link me tp the upstream discussion of this problem please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened an issue to track the flit install -s
problem. It's at: pypa/flit#394, but I figure it might get merged with the wheel name issue since they seem related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this plus the change to sphinx
pinning and this should be good to go.
Also, release note if you want?
Is the anndata PR up to date as well?
Then, feel free to merge this.
🎆 🎆 🎆
Let’s wait for #1700 /edit: done! |
OK, merging! Let’s do anndata ASAP then so people don’t have two different dev environments for too long. |
Had to restore the branch to reopen #1700 (and switch its base to master) |
What stays the same:
pip install scanpy
pip install .
pip install git+https://...
What changes:
pip install -e .[every,single,extra]
→flit install -s
for dev installsbeni pyproject.toml > environment.yml
for condaflit build
andflit publish
. Maybe installkeyring
to store your publish password, and you know everything you need to.flit build
doesn’t clutter your dev directory withbuild/
and*.egg-info/
junk, it just createsdist/scanpy-*{.whl,.tar.gz}
.