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

Fix tests #552

Merged
merged 9 commits into from
Aug 18, 2024
Merged

Fix tests #552

merged 9 commits into from
Aug 18, 2024

Conversation

beenje
Copy link
Contributor

@beenje beenje commented Aug 17, 2024

Description

Some tests are currently broken on main. Trying to fix those:

  • fix test_short_license_id (DEC-3-Clause' detected instead of BSD-3-Clause`) - thanks to @dholth (commit from Generate recipe from project tree using pypa/build #541)
  • fix --pypi-url: always raised a RuntimeError (args.url_pypi_metadata has a default value and will never be empty)
  • fix meta.yaml output directory (the meta.yaml file was always created in the current directory instead of recipe_dir)
  • fix test_poetry_langchain_snapshot: replace pypi.io with pypi.org (pypi.org is the official domain)
  • skip test_console_script_toml_format for python 3.12 right now. We need to find another package for that test. Do you know any?
  • fix test_merge_pypi_sdist_metadata for python 3.12: bump gsw version
  • fix test_injection_distutils_compiler_gsw for python 3.12: bump gsw version - note that packages can't be retrieved from pyproject.toml right now
  • fix test_ciso_recipe for python 3.12: bump ciso version

dholth and others added 4 commits August 17, 2024 10:43
args.url_pypi_metadata has a default value and will never be empty.
To check if both options aren't given, we have to check if it's different
fro the default value.
A small bug was introduced when adding v1 format.
The "meta.yaml" file was always created in the current directory instead of recipe_dir.
pypi.org is the official domain.

See pypi/warehouse#2184
@beenje beenje requested a review from a team as a code owner August 17, 2024 12:14
@beenje beenje marked this pull request as draft August 17, 2024 12:14
@beenje
Copy link
Contributor Author

beenje commented Aug 17, 2024

  • test_injection_distutils_compiler_gsw is failing on python 3.12 due to:
    DEBUG grayskull.strategy.py_base:py_base.py:460 Exception when executing setup.py as script: module 'configparser' has no attribute 'SafeConfigParser'

SafeConfigParser was removed in 3.12.
This comes from versioneer.py included in the sdist. Test should be updated to use another package/version.

  • test_merge_pypi_sdist_metadata: same issue as above as it's using the same package (gsw 3.3.1).

  • test_ciso_recipe: same issue as above. ciso-0.1.0 is also using versioneer.py

  • test_console_script_toml_format fails with DEBUG grayskull.strategy.py_base:py_base.py:447 When executing setup.py did not find the module: imp. Exception: No module named 'imp'. Module imp used in setup.py was removed in Python 3.12 - consolemd-0.5.1 not compatible with 3.12

Fix: Mixing implicit and explicit returns may indicate an error as implicit returns always return None
Bump version to 0.2.2.
Version 0.1.0 includes versioneer.py which isn't compatible with Python 3.12
(using SafeConfigParser).
Bump gsw version to 3.6.19.
Version 3.3.1 includes versioneer.py which tries to import SafeConfigParser
that was removed in Python 3.12.

"packages" key isn't detected anymore as most metadata were moved
to pyproject.toml. Only C extension definition is left in setup.py.
Bump gsw version to 3.6.19.
Version 3.3.1 includes versioneer.py which tries to import SafeConfigParser
that was removed in Python 3.12.
Package setup.py not compatible with 3.12 (try to import imp).
New package to find for this test. Skip for now.
[
"cython >=3",
"numpy >=2.0.0rc1",
"oldest-supported-numpy",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't have both numpy >=2.0.0rc1 and oldest-supported-numpy but it looks like grayskull doesn't support selectors?

Copy link
Member

Choose a reason for hiding this comment

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

which selectors are you talking about? It does support a few of them but in this case they might not be being generated for some reason

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is requirements/host. Don't think we want oldest-supported-numpy here

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

that equal over there is just a syntax sugar, but if you properly compare the numpy and oldest-supported-numpy you will see the selectors :)

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, looking at the generated meta.yaml, I see:

    - oldest-supported-numpy  # [py<39]
    - numpy >=2.0.0rc1  # [py>=39]

@beenje beenje marked this pull request as ready for review August 18, 2024 20:15
@marcelotrevisani
Copy link
Member

thanks for fixing the tests, much appreciated! :)

@marcelotrevisani marcelotrevisani merged commit 391b7cc into conda:main Aug 18, 2024
7 checks passed
)
assert sorted(recipe["requirements"]["run"]) == sorted(
["cython", "python", "<{ pin_compatible('numpy') }}"]
["<{ pin_compatible('numpy') }}", "oldest-supported-numpy", "python >=3.9"]
Copy link
Member

Choose a reason for hiding this comment

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

Also not sure why oldest-supported-numpy is added to requirements/run

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jakirkham , at first sight it seems odd, indeed, but it comes directly from the requirements in that package gsw, it is not something that grayskull adds,
https://github.com/TEOS-10/GSW-Python/blob/c3407368ef01d0b3d8e85f8cf7b94abae52bcd25/pyproject.toml#L6

that is a separated package on pypi
https://pypi.org/project/oldest-supported-numpy/

Copy link
Contributor Author

@beenje beenje Aug 19, 2024

Choose a reason for hiding this comment

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

Indeed this is actually for the ciso project that has oldest-supported-numpy not only in build-system requirements , but in dependencies as well:

dependencies = [
    "oldest-supported-numpy ; python_version < '3.9'",
    "numpy>=2.0.0rc1 ; python_version >= '3.9'",
]

gsw has only numpy>=1.21 in dependencies (oldest-supported-numpy is only in the build-system).
oldest-supported-numpy doesn't appear the run requirements in that case.

Copy link
Member

Choose a reason for hiding this comment

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

It's ok to have in pyproject.toml

However what this appears to be testing is whether this is added to requirements/host and requirements/run. The latter doesn't make sense

Copy link
Member

Choose a reason for hiding this comment

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

it is going to appear in the requirements run because oldest-supported-numpy is specified in the dependencies section of the pyproject.toml. If it is was only in build section of pyproject.toml that would appear only in host then.

It is possible to do a workaround and always remove it from the requirements/run and always let it in host, but I am just not so sure if it covers all scenarios, what do you think?

Copy link
Member

@jakirkham jakirkham Aug 20, 2024

Choose a reason for hiding this comment

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

We should remove oldest-supported-numpy whenever it is seen. That's used by wheels specifically to configure builds with specific numpy versions (not sure why a wheel would use oldest-supported-numpy at runtime unless they are some kind of build tool)

In any event it doesn't make sense in the Conda case where we configure NumPy versions used in some conda_build_config.yaml or other variants

On the Conda side, we need only add numpy (unconstrained) to requirements/host and copy over any dependencies value of numpy to requirements/run

Copy link
Member

Choose a reason for hiding this comment

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

ok, gotcha
That is pretty much a new PR to do this workaround as it was not a requirement before.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha thanks for the context! 🙏

The issue is oldest-supported-numpy doesn't exist in Conda. So if Grayskull adds it to recipes, they will be broken. Maintainers will need to know they can clean this out to fix the issue

In any event, we can discuss more in the new issue (thanks for raising! 🙏): #553

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

Successfully merging this pull request may close these issues.

4 participants