Run upstream test suite #55

h-vetinari · 2022-10-24T00:34:32Z

Pick up unmerged improvements from #40

needs data downloads & extra requirements, see https://github.com/huggingface/tokenizers/blob/python-v0.13.1/bindings/python/setup.py#L5

conda-forge-linter · 2022-10-24T00:34:36Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

h-vetinari · 2022-10-24T00:42:05Z

Not sure why this is picking up openssl3 already, which depends on arrow being migrated.

…nda-forge-pinning 2022.10.23.10.16.33

…nda-forge-pinning 2022.10.24.01.16.58

h-vetinari · 2022-10-24T05:03:23Z

@conda-forge/tokenizers
I worked a bunch to get python 3.10 to work (i.e. huggingface/tokenizers#956), but it took a while for a new release to materialise. Now that #53 is in, I wanted to pick up the improvements from #40 again, and am willing to add myself as a maintainer (also happy to take care of the OpenSSL 3 migration).

PTAL 🙃

setu4993

Thanks, @h-vetinari! This makes sense to me. 1 question (inline) to confirm why we're switching from PyPI to GitHub, but the rest looks good to me.

Happy to have you help maintain this package :).

setu4993 · 2022-10-25T06:53:31Z

recipe/meta.yaml

  version: {{ version }}

 source:
-  url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
-  sha256: 3333d1cee5c8f47c96362ea0abc1f81c77c9b92c6c3d11cbf1d01985f0d5cf1d
+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz


Is the goal to switch from PyPI to GitHub so that we can include and run the upstream test suite?

Yes, because the tests are generally not part of the tarball (I don't specifically remember if I tested this for this feedstock, but I consider it cleaner than PyPI sources because there's one less layer where stuff can be spuriously changed).

Yeah, makes sense.

setu4993 · 2022-10-25T06:53:47Z

recipe/meta.yaml

-{% set name = "tokenizers" %}
 {% set version = "0.13.1" %}

 package:
-  name: {{ name|lower }}
+  name: tokenizers


Can we keep these, please?

I'm not a fan of these, because it's a "fake" variable - it can never be changed. It may help for the initial recipe generation, but serves no purpose afterward, and makes reading the recipe (and copying the source URL, e.g. for calculating a new hash) harder. If you insist, I'll change it, just very reluctantly 🙃

setu4993 · 2022-10-25T06:54:15Z

recipe/meta.yaml

+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz
+  sha256: 41cff8c8c87ba6dfbd9eb1d89b006aabb9c9823ffd09e281d6ddfb9ae695bd1a
+  patches:
+    - patches/0001-don-t-fork-on-windows.patch  # [win]


Nit:

Suggested change

- patches/0001-don-t-fork-on-windows.patch # [win]

- patches/0001-dont-fork-on-windows.patch # [win]

This is generated from the git commit messages with git format-patch, it makes no sense to police these IMO (though if you want I can change the commit message of the patch).

Not policing it, just recommending what I think is cleaner. But that's fine.

setu4993 · 2022-10-25T06:55:16Z

recipe/meta.yaml

    - {{ PYTHON }} -m pip install . -vv

 requirements:
  build:
    - python                                 # [build_platform != target_platform]
    - cross-python_{{ target_platform }}     # [build_platform != target_platform]
+    - openssl                                # [build_platform != target_platform]


Do we need to pin the version here?

No, that's taken care of by the global pinning. :)

setu4993 · 2022-10-25T06:55:39Z

recipe/meta.yaml

  requires:
    - pip
+    - pytest
+    - datasets
+    - numpy *


I hadn't seen the * notation in a recipe, TIL :).

It keeps conda smithy from spuriously thinking it needs to compile the recipe against different numpy versions. :)

setu4993 · 2022-10-25T06:56:01Z

recipe/meta.yaml

+    # adapted from https://github.com/huggingface/tokenizers/blob/master/bindings/python/Makefile
+    - mkdir data
+    - curl https://norvig.com/big.txt > data/big.txt
+    {% set tests_to_skip = "_not_a_real_test" %}
+    # windows and expectation of forking -> not gonna happen
+    {% set tests_to_skip = tests_to_skip + " or with_parallelism" %}  # [win]
+    - pytest -v tests -k "not ({{ tests_to_skip }})"


h-vetinari

Thanks for the review! :)

h-vetinari · 2022-10-25T07:19:52Z

recipe/meta.yaml

  version: {{ version }}

 source:
-  url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
-  sha256: 3333d1cee5c8f47c96362ea0abc1f81c77c9b92c6c3d11cbf1d01985f0d5cf1d
+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz


Yes, because the tests are generally not part of the tarball (I don't specifically remember if I tested this for this feedstock, but I consider it cleaner than PyPI sources because there's one less layer where stuff can be spuriously changed).

h-vetinari · 2022-10-25T07:22:15Z

recipe/meta.yaml

-{% set name = "tokenizers" %}
 {% set version = "0.13.1" %}

 package:
-  name: {{ name|lower }}
+  name: tokenizers


I'm not a fan of these, because it's a "fake" variable - it can never be changed. It may help for the initial recipe generation, but serves no purpose afterward, and makes reading the recipe (and copying the source URL, e.g. for calculating a new hash) harder. If you insist, I'll change it, just very reluctantly 🙃

h-vetinari · 2022-10-25T07:23:09Z

recipe/meta.yaml

+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz
+  sha256: 41cff8c8c87ba6dfbd9eb1d89b006aabb9c9823ffd09e281d6ddfb9ae695bd1a
+  patches:
+    - patches/0001-don-t-fork-on-windows.patch  # [win]


This is generated from the git commit messages with git format-patch, it makes no sense to police these IMO (though if you want I can change the commit message of the patch).

h-vetinari · 2022-10-25T07:23:25Z

recipe/meta.yaml

    - {{ PYTHON }} -m pip install . -vv

 requirements:
  build:
    - python                                 # [build_platform != target_platform]
    - cross-python_{{ target_platform }}     # [build_platform != target_platform]
+    - openssl                                # [build_platform != target_platform]


No, that's taken care of by the global pinning. :)

h-vetinari · 2022-10-25T07:24:01Z

recipe/meta.yaml

  requires:
    - pip
+    - pytest
+    - datasets
+    - numpy *


It keeps conda smithy from spuriously thinking it needs to compile the recipe against different numpy versions. :)

h-vetinari · 2022-10-27T21:47:30Z

@setu4993
Are you satisfied with my answers or could you give this another look please? :)

setu4993

Looks great, @h-vetinari! Thanks for contributing and glad to have you maintain this alongside.

setu4993 · 2022-10-28T03:36:40Z

recipe/meta.yaml

  version: {{ version }}

 source:
-  url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
-  sha256: 3333d1cee5c8f47c96362ea0abc1f81c77c9b92c6c3d11cbf1d01985f0d5cf1d
+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz


Yeah, makes sense.

setu4993 · 2022-10-28T03:37:12Z

recipe/meta.yaml

+  url: https://github.com/huggingface/tokenizers/archive/refs/tags/python-v{{ version }}.tar.gz
+  sha256: 41cff8c8c87ba6dfbd9eb1d89b006aabb9c9823ffd09e281d6ddfb9ae695bd1a
+  patches:
+    - patches/0001-don-t-fork-on-windows.patch  # [win]


Not policing it, just recommending what I think is cleaner. But that's fine.

setu4993 · 2022-10-28T03:37:40Z

recipe/meta.yaml

  requires:
    - pip
+    - pytest
+    - datasets
+    - numpy *


setu4993 · 2022-10-28T03:40:25Z

Sorry it took a few days.

h-vetinari · 2022-10-28T03:55:14Z

Sorry it took a few days.

No worries! :)

Thanks for the review & merging!

h-vetinari · 2022-10-28T03:57:43Z

Just saw that this broke due to a pretty banal pip check...

+ pip check
multiprocess 0.70.14 has requirement dill>=0.3.6, but you have dill 0.3.5.1.

I think I'll fix it directly in #56

h-vetinari added 4 commits October 24, 2022 11:28

fix cross-compilation issues

d90b5e5

switch to github sources (to enable running test suite)

edae3d2

run upstream test suite

a8e1856

needs data downloads & extra requirements, see https://github.com/huggingface/tokenizers/blob/python-v0.13.1/bindings/python/setup.py#L5

add myself to maintainers

1904a57

h-vetinari requested review from Anthchirp, ndmaxar, oblute and setu4993 as code owners October 24, 2022 00:34

h-vetinari force-pushed the tests branch from fb42c20 to b1939f0 Compare October 24, 2022 01:07

h-vetinari mentioned this pull request Oct 24, 2022

add pybcj to aarch migration conda-forge/conda-forge-pinning-feedstock#3566

Merged

h-vetinari added 6 commits October 24, 2022 14:56

skip parallel tests on windows

ed4c09d

skip openssl 3 for now

9c84709

don't fork on windows

402c088

MNT: Re-rendered with conda-build 3.22.0, conda-smithy 3.21.2, and co…

eed4aaa

…nda-forge-pinning 2022.10.23.10.16.33

cross-compile aarch64/ppc64le

f75825a

MNT: Re-rendered with conda-build 3.22.0, conda-smithy 3.21.2, and co…

1bf282a

…nda-forge-pinning 2022.10.24.01.16.58

h-vetinari force-pushed the tests branch from b1939f0 to 1bf282a Compare October 24, 2022 04:11

h-vetinari mentioned this pull request Oct 24, 2022

Arch Migrator conda-forge/pybcj-feedstock#16

Merged

h-vetinari mentioned this pull request Oct 24, 2022

Rebuild for openssl3 #48

Merged

setu4993 reviewed Oct 25, 2022

View reviewed changes

h-vetinari commented Oct 25, 2022

View reviewed changes

h-vetinari mentioned this pull request Oct 27, 2022

Rebuild for python311 #56

Merged

setu4993 approved these changes Oct 28, 2022

View reviewed changes

setu4993 merged commit 628815d into conda-forge:main Oct 28, 2022

h-vetinari deleted the tests branch October 28, 2022 03:43

h-vetinari mentioned this pull request Oct 28, 2022

fix requirements for dill conda-forge/multiprocess-feedstock#46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run upstream test suite #55

Run upstream test suite #55

h-vetinari commented Oct 24, 2022

conda-forge-linter commented Oct 24, 2022

h-vetinari commented Oct 24, 2022

h-vetinari commented Oct 24, 2022

setu4993 left a comment

setu4993 Oct 25, 2022

h-vetinari Oct 25, 2022

setu4993 Oct 28, 2022

setu4993 Oct 25, 2022

h-vetinari Oct 25, 2022

setu4993 Oct 25, 2022

h-vetinari Oct 25, 2022

setu4993 Oct 28, 2022

setu4993 Oct 25, 2022

h-vetinari Oct 25, 2022

setu4993 Oct 25, 2022

h-vetinari Oct 25, 2022

setu4993 Oct 28, 2022

setu4993 Oct 25, 2022

h-vetinari left a comment

h-vetinari Oct 25, 2022

h-vetinari Oct 25, 2022

h-vetinari Oct 25, 2022

h-vetinari Oct 25, 2022

h-vetinari Oct 25, 2022

h-vetinari commented Oct 27, 2022

setu4993 left a comment

setu4993 Oct 28, 2022

setu4993 Oct 28, 2022

setu4993 Oct 28, 2022

setu4993 commented Oct 28, 2022

h-vetinari commented Oct 28, 2022

h-vetinari commented Oct 28, 2022

	- patches/0001-don-t-fork-on-windows.patch # [win]
	- patches/0001-dont-fork-on-windows.patch # [win]

Run upstream test suite #55

Run upstream test suite #55

Conversation

h-vetinari commented Oct 24, 2022

conda-forge-linter commented Oct 24, 2022

h-vetinari commented Oct 24, 2022

h-vetinari commented Oct 24, 2022

setu4993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented Oct 27, 2022

setu4993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setu4993 commented Oct 28, 2022

h-vetinari commented Oct 28, 2022

h-vetinari commented Oct 28, 2022