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

prototype implementation of interpreting PEP725 metadata #518

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

msarahan
Copy link
Contributor

@msarahan msarahan commented Jan 4, 2024

In https://discuss.python.org/t/pep-725-specifying-external-dependencies-in-pyproject-toml/31888/40, I offered to prototype an implementation of PEP725 for Grayskull. Here we are!

PEP 725 is the PEP to allow expression of external dependencies in pyproject.toml. It basically boils down to a grand scheme to unify a translation table across the many ecosystems. This PR should be interpreted in the context of the discussion at https://discuss.python.org/t/pep-725-specifying-external-dependencies-in-pyproject-toml/31888

This prototype demonstrates the name mapping, but this won't be a real implementation until a definitive name map is created for conda-forge (and perhaps elsewhere).

I have taken the liberty of consolidating test_flit.py and test_poetry.py into test_py_toml.py, because I found the difference between the implementation in the one file (py_toml.py) vs the split-up test implementation to make things harder to find.

CC @rgommers

Update: after all revisions and fumbling, the design here is:

  1. extract external dependencies from pyproject.toml
  2. Merge these external dependencies with setup.py/setup.cfg metadata. The build dependencies don't have an equivalent in setup.py/setup.cfg, so we make a separate placeholder for them.
  3. get the merged dependencies for the conda recipe requirements
  4. If any PURL patterns remain in the generated recipe, they are highlighted in yellow. This yellow color is added to the key

I think we might want to set a non-zero return code if there are any dependencies not found or PURLs not mapped, but that would be a behavior change. I did not want to get into that with this PR.

@msarahan msarahan requested a review from a team as a code owner January 4, 2024 21:17
@rgommers
Copy link

rgommers commented Jan 5, 2024

Thanks @msarahan! This looks nicely straightforward.

I tried some initial testing, but am running into an unrelated issue with grayskull not understanding non-distutils packages. E.g., for numpy and scipy I see:

Checking for pyproject.toml
pyproject.toml found in /tmp/grayskull-numpy-e0nj9egc/numpy-2.0.0.dev0/pyproject.toml
Recovering information from setup.py
Executing injected distutils...
...

Followed by an exception (for scipy) or it trying to build and run Cython somehow and crashing (for numpy).

I don't have time to figure out what's up with that right now - are there other packages on which you successfully ran grayskull with this PR?

I'm not even sure why grayskull would try and build the package - clearly that isn't going to work when there are external dependencies, unless it already parsed [external] and installed those deps with conda.

@msarahan
Copy link
Contributor Author

msarahan commented Jan 5, 2024

To be honest, I designed this only as proof of concept of reading the pyproject.toml tags and translating them into conda recipe equivalents. My tests were limited to unit tests of that translation process. Grayskull can be pointed at local sdists, though, so I will expand this to adopt your full integration testing with building packages from external-deps-build.

@rgommers
Copy link

rgommers commented Jan 5, 2024

Thanks Mike, it'd be great to see it work on an sdist indeed.

I have only used grayskull a handful of times, so I may be missing something about what it's supposed to do or how to not make it try to build the package.

@msarahan
Copy link
Contributor Author

msarahan commented Jan 5, 2024

This goes much deeper than I thought. The translation happens fine, but grayskull's later parts don't pass it through. They go to setup.py/.cfg schema before really going to conda. It seems to me that we have to have a more direct path from pyproject.toml, and maintain the legacy path without pyproject.toml

@marcelotrevisani can you give me any context on why you chose to go from pyproject -> setup_py dict -> conda recipe? Was is just avoiding duplicating some stuff to have

pyproject -> conda
setup_py -> conda

@marcelotrevisani
Copy link
Member

This goes much deeper than I thought. The translation happens fine, but grayskull's later parts don't pass it through. They go to setup.py/.cfg schema before really going to conda. It seems to me that we have to have a more direct path from pyproject.toml, and maintain the legacy path without pyproject.toml

@marcelotrevisani can you give me any context on why you chose to go from pyproject -> setup_py dict -> conda recipe? Was is just avoiding duplicating some stuff to have

pyproject -> conda
setup_py -> conda

it is because there are some cases where the pyproject.toml is present but the dependencies/metadata are not all specified there. Take pytest as example, there is a pyproject.toml there, however, the metadata is in a setup.cfg

@msarahan
Copy link
Contributor Author

msarahan commented Jan 5, 2024

Thanks. That's a bummer, but I'm grateful for the quick response and pointer. I'll keep poking at it.

@marcelotrevisani
Copy link
Member

I have seen a few cases where people maintain both, also other cases where part of the metadata is in a pyproject.toml and part in a setup.cfg file. well, chaos 😆

@msarahan
Copy link
Contributor Author

msarahan commented Jan 5, 2024

Where's the poop emoji when you need it?

@marcelotrevisani
Copy link
Member

if you need any help from my side, please let me know. :)

@rgommers
Copy link

rgommers commented Jan 5, 2024

I have seen a few cases where people maintain both, also other cases where part of the metadata is in a pyproject.toml and part in a setup.cfg file.

You can derive this quite easily from the pyproject.toml content though. The one for pytest doesn't have a [project] table, so there are no dependencies there. However, if there is a [project] table and dependencies under it is not marked as dynamic, you can simply parse the pyproject.toml contents and be done with that.

There may be other content you need; the only one that's often dynamic is version (but you have it in the sdist already). Having the default path not execute a build would be really helpful I'd think, and more generic.

@marcelotrevisani
Copy link
Member

I have seen a few cases where people maintain both, also other cases where part of the metadata is in a pyproject.toml and part in a setup.cfg file.

You can derive this quite easily from the pyproject.toml content though. The one for pytest doesn't have a [project] table, so there are no dependencies there. However, if there is a [project] table and dependencies under it is not marked as dynamic, you can simply parse the pyproject.toml contents and be done with that.

There may be other content you need; the only one that's often dynamic is version (but you have it in the sdist already). Having the default path not execute a build would be really helpful I'd think, and more generic.

Yeah, that is something that I am working on a side project that I plan to integrate with grayskull with other bits, but it might take a while... :/

It is possible to add an option to give priority for pyproject.toml and get everything from it and not run the setup

@msarahan
Copy link
Contributor Author

msarahan commented Jan 5, 2024

I got a successful build with a skeleton generated from cffi. This stuff still needs polish before the PR is really ready. I have this code outputting a comment when a match can't be made for the PURL. I think that's nice, but I putting in comments is new in Grayskull, and it's glitchy. For example,

requirements:
  build:
    - "{{ compiler('c') }}"
  host:
    - python  #
# warning: no matching mapping was found for purl "pkg:generic/libffi". the dependency has been omitted here.
    - pip

Here the trailing comment after the python dependency shouldn't be here, and the indentation of the comment is wrong. he The indentation doesn't break the build, but it definitely doesn't look right.

Anyway, more to come.

if pkg_name in PIN_PKG_COMPILER.keys():
requirements["run"] = clean_list_pkg(pkg_name, requirements["run"])
requirements["run"].append(PIN_PKG_COMPILER[pkg_name])
pkg_name_match = RE_DEPS_NAME.match(pkg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this change is that with PURLs, the deps name regex may not match. If that happens, then we are trying to call None.group(0) which of course fails.

@msarahan
Copy link
Contributor Author

msarahan commented Jan 10, 2024

@conda/grayskull I think this is ready for some review/discussion. Please check the original PR text for an updated summary of what's going on here. I ditched the comment-for-missing-PURLs approach mentioned above, and went with Grayskull's existing mechanism for showing missing dependencies.

@marcelotrevisani
Copy link
Member

Hi @msarahan , thanks for that! I am planning to review this over the weekend.

@marcelotrevisani
Copy link
Member

Sorry for my delay, the initial support looks good to me actually :)
Thanks for adding support for this

@marcelotrevisani marcelotrevisani merged commit 312ad3a into conda:main Jan 22, 2024
9 checks passed
@msarahan msarahan deleted the pep725-prototype branch January 23, 2024 20:14
@msarahan
Copy link
Contributor Author

Thanks @marcelotrevisani. To clarify what comes next:

If you see problems related to this new PEP725 stuff, please ping me. I'll be happy to help maintain this.

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.

3 participants