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.

Sign up for GitHub

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

Jump to bottom

Spacy #36899

Merged
merged 6 commits into from Mar 14, 2018
Merged

Spacy #36899

merged 6 commits into from Mar 14, 2018

Conversation

aborsu
Copy link
Contributor

@aborsu aborsu commented Mar 13, 2018 •
edited

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@aborsu aborsu requested a review from FRidh as a code owner March 13, 2018 06:45
@aborsu
Copy link
Contributor Author

aborsu commented Mar 13, 2018

I still need to add the update to spacy.
This is to fix python.thinc which is broken on master.

@hedning
Copy link
Contributor

hedning commented Mar 13, 2018

You need to add yourself to maintainers/maintainer-list.nix so that lib.maintainers.aborsu is defined (that's what ofborg is complaining about).

@aborsu
Copy link
Contributor Author

aborsu commented Mar 13, 2018

Fix thinc and spacy for #36453

dotlambda
dotlambda requested changes Mar 14, 2018
View reviewed changes
pkgs/development/python-modules/spacy/default.nix Outdated
};

preBuild = ''
Copy link
Member

Choose a reason for hiding this comment

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

prePatch

pkgs/development/python-modules/thinc/default.nix Outdated
];

preBuild = ''
Copy link
Member

Choose a reason for hiding this comment

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

prePatch

pkgs/development/python-modules/ftfy/default.nix
pname = "ftfy";
# latest is 5.1.1, buy spaCy requires 4.4.3
Copy link
Member

Choose a reason for hiding this comment

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

Why did you drop the comment?
Please open a PR at the spaCy side that allows it to use a newer ftfy version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can open a PR but I very much doubt that it will be accepted.
Spacy is still very much supporting python2.7 and ftfy v5 dropped support for python3.
The v4 branch of ftfy still gets some updates so I doubt the maintainer of Spacy will accept working with two different versions of ftfy as that would make keeping the 2.7 compatibility that much harder.

Copy link
Member

Choose a reason for hiding this comment

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

As long as nobody needs a newer version of ftfy, I think it's okay to stay at the old one. However, please keep the comment.

pkgs/development/python-modules/msgpack-numpy/default.nix Outdated
numpy
];

doCheck = true;
Copy link
Member

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? There are tests, they work an they take less than 10ms to run?

Copy link
Member

Choose a reason for hiding this comment

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

doCheck is set to true by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... indeed

Augustin Borsu added 6 commits March 14, 2018 20:29
python.msgpack-numpy: init 0.4.1
5568e25
python.murmurhash: 0.26.4 -> 0.28.0
2e67c8e
python.thinc: 6.5.1 -> 6.10.2
9478948
add aborsu as maintainer
458ba30
python.ftfy: 5.3.0 -> 4.4.3
8187d93 
V5 only supports python3. Since at the moment the only packages
that use ftfy are spacy and textacy which both support
python2 and 3, I propose to roll back to v4 until another package
requires v5, at that point we can make a duplicate package.
python.spacy: 1.8.2 -> 2.0.9
22fdb0b
@aborsu
Copy link
Contributor Author

aborsu commented Mar 14, 2018

@dotlambda I have made the requested changes.

@dotlambda
Copy link
Member

@GrahamcOfBorg build python2.pkgs.msgpack-numpy python3.pkgs.msgpack-numpy python2.pkgs.murmurhash python3.pkgs.murmurhash python2.pkgs.thinc python3.pkgs.thinc python2.pkgs.ftfy python3.pkgs.ftfy python2.pkgs.spacy python3.pkgs.spacy

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: python2.pkgs.msgpack-numpy, python3.pkgs.msgpack-numpy, python2.pkgs.murmurhash, python3.pkgs.murmurhash, python2.pkgs.thinc, python3.pkgs.thinc, python2.pkgs.ftfy, python3.pkgs.ftfy, python2.pkgs.spacy, python3.pkgs.spacy

Partial log (click to expand)

/nix/store/8p2frarb887kbd2l2pqw7vkfkc691r69-python2.7-msgpack-numpy-0.4.1
/nix/store/60k4zv5k8wpfw84chsjmifh8dp7la2vm-python3.6-msgpack-numpy-0.4.1
/nix/store/kg4kssf85a045pj4fp00skdavr0cdd91-python2.7-murmurhash-0.28.0
/nix/store/hi14f7n56c6w1jlx0s9jzwgav6kf63id-python3.6-murmurhash-0.28.0
/nix/store/x6dzr8vmw8j94faqny1i945kb7v37ij6-python2.7-thinc-6.10.2
/nix/store/nv8fbgj4315pyqkn009z4jhwn3raia3j-python3.6-thinc-6.10.2
/nix/store/gx23rc5iwzsrv9142xizzcayg8jjj2vz-python2.7-ftfy-4.4.3
/nix/store/0n6jhisnavr9xxm1ghv8lkipp7cf6icr-python3.6-ftfy-4.4.3
/nix/store/wwdrzvgygpgxahf1rr5qyxjlb328phx2-python2.7-spacy-2.0.9
/nix/store/f2igd9wip42jqazjssni8d2hwfl6pwxv-python3.6-spacy-2.0.9

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: python2.pkgs.msgpack-numpy, python3.pkgs.msgpack-numpy, python2.pkgs.murmurhash, python3.pkgs.murmurhash, python2.pkgs.thinc, python3.pkgs.thinc, python2.pkgs.ftfy, python3.pkgs.ftfy, python2.pkgs.spacy, python3.pkgs.spacy

Partial log (click to expand)

/nix/store/a8m0jxc94qnql0g0i5lgdqwv8hf9k0cz-python2.7-msgpack-numpy-0.4.1
/nix/store/41c5qafq4avpn59sp1asfnpy3bxc9dbf-python3.6-msgpack-numpy-0.4.1
/nix/store/5z5jbhimgkghl81fbl1f41yg2c15s47r-python2.7-murmurhash-0.28.0
/nix/store/rlc4cg5cn4afa3bw2qcd7l80qg23mbf1-python3.6-murmurhash-0.28.0
/nix/store/ri3mmr3xjw3jni3j1jl6v6brydmmf6in-python2.7-thinc-6.10.2
/nix/store/ffhrzipq8v4z38br4j80j84c6lg6h2aj-python3.6-thinc-6.10.2
/nix/store/kljyqkng0jihgrhgzq58m04pvq8j1l0r-python2.7-ftfy-4.4.3
/nix/store/d4dkmzd85appmvk9rqq3qplpsxwkkw6f-python3.6-ftfy-4.4.3
/nix/store/if81f48mh9zf95ixvz2yl7k6q4d5rqjw-python2.7-spacy-2.0.9
/nix/store/ldin5b4dj6j1d234a4rgzdxji9vnrhfx-python3.6-spacy-2.0.9

@dotlambda dotlambda merged commit d78a411 into NixOS:master Mar 14, 2018
@dotlambda
Copy link
Member

@aborsu Btw, ftfy will probably be upgraded again with the next automatic package update. Thinking about it, we probably should just have disabled spacy for Python 3. Maybe you can find out if spacy works with ftfy v5 in the meantime. At least, it will work fine for 18.03 now.

@aborsu
Copy link
Contributor Author

aborsu commented Mar 15, 2018 •
edited

@dotlambda I would be saddened if spacy was disabled on python3 as I personally don't use python2 anymore. What about creating a ftfy_v4 package alongside the ftfy package?

@dotlambda
Copy link
Member

Oh sorry, it's the other way round. Ftfy v5 only works with Python 3.
pythonPackages should be a consistent set of packages. Therefore, we usually do not want multiple versions of the same package in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aborsu @hedning @dotlambda @GrahamcOfBorg