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

[DRAFT] Add version to metadata hook, use version to retrieve deps #1511

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

potiuk
Copy link
Contributor

@potiuk potiuk commented May 20, 2024

This is a very draft proposal on how to fix #1348 and #1349 - more
to see if this is a good direction. It should likely be split to
two PRs (and of course tests and docs are needed):

  • extending custom metadata plugin to allow passing version through
    hook's version property to distinguish standard and editable builds
    (also including extending the hatchling CLIs.

  • adding version to CLI where hatch queries hatchling to include standard/
    editable version when querying for available dependencies.

Not all changes have been yet applied, this is more to check if
this is the right direction.

@potiuk
Copy link
Contributor Author

potiuk commented May 20, 2024

cc: @ofek -> would love to get some comments on that.

@ofek
Copy link
Collaborator

ofek commented May 23, 2024

Instead, what do you think about adding a context manager on the metadata class for temporarily setting a property which could be used here? https://github.com/pypa/hatch/blob/hatchling-v1.24.2/backend/src/hatchling/builders/plugin/interface.py#L89-L90

@potiuk
Copy link
Contributor Author

potiuk commented May 27, 2024

Instead, what do you think about adding a context manager on the metadata class for temporarily setting a property which could be used here? https://github.com/pypa/hatch/blob/hatchling-v1.24.2/backend/src/hatchling/builders/plugin/interface.py#L89-L90

Yes. it will remove the need to have a new hook method if we do that, I was a bit hesitant to do that as the "metadata" will be quite a bit overloaded in this case (version is not technically core metadata, more of a transient state of the build) - but since then I also realised that we already have some things there that are already more than metadata (for example entrypoints are also available there), so if you are ok with this - I am happy to rework it this way.

@potiuk potiuk force-pushed the add-version-to-update-metadata-hook branch from f86ef9a to 24874f3 Compare May 27, 2024 21:08
@potiuk
Copy link
Contributor Author

potiuk commented May 27, 2024

Smth like that ?

@potiuk potiuk force-pushed the add-version-to-update-metadata-hook branch from 24874f3 to 328e06f Compare May 27, 2024 21:10
@@ -193,8 +203,9 @@ def core(self) -> CoreMetadata:

if metadata.dynamic:
for metadata_hook in metadata_hooks.values():
metadata_hook.update(self.core_raw_metadata)
metadata.add_known_classifiers(metadata_hook.get_known_classifiers())
with set_version(metadata_hook, self.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.

Here the context manager - and we have version property in the hook which we set (and can query in the hook).

This is a very draft proposal on how to fix pypa#1348 and pypa#1349 - more
to see if this is a good direction. It should likely be split to
two PRs (and of course tests and docs are needed):

* extending custom metadata plugin to allow passing version through
  hook's version property to distinguish standard and editable builds
  (also including extending the hatchling CLIs.

* adding version to CLI where hatch queries hatchling to include standard/
  editable version when querying for available dependencies.

Not all changes have been yet applied, this is more to check if
this is the right direction.
@potiuk potiuk force-pushed the add-version-to-update-metadata-hook branch from 328e06f to 762d7a4 Compare May 27, 2024 21:13
@potiuk
Copy link
Contributor Author

potiuk commented May 27, 2024

Or did you think to bubble it up to BuilderInterface ?

@ofek
Copy link
Collaborator

ofek commented May 27, 2024

Upon closer inspection I think the context manager I was thinking of would go on the first line after this loop and everything under would be indented: https://github.com/pypa/hatch/blob/hatchling-v1.24.2/backend/src/hatchling/builders/plugin/interface.py#L136

The context manager would live directly on the metadata class and would be used on self.metadata. Metadata hooks could then read the version and if the version is unset i.e. None, then that would indicate that no build is happening but we are trying to query metadata directly.

Does that make sense? I haven't thoroughly looked at the moving parts but I think that would work, please let me know what you think!

edit: I just thought of something...

This line would then need to be inside the loop and within the context manager https://github.com/pypa/hatch/blob/hatchling-v1.24.2/backend/src/hatchling/builders/plugin/interface.py#L90 but then it wouldn't work when building multiple versions because the first invocation triggers metadata hooks. Hmm, since building multiple versions of a target at once is only possible with Hatch (and really not useful for wheels/source distributions) maybe let's forgo purity and send the list of versions here https://github.com/pypa/hatch/blob/hatchling-v1.24.2/backend/src/hatchling/builders/plugin/interface.py#L83 directly to the validate_fields method.

@potiuk
Copy link
Contributor Author

potiuk commented May 27, 2024

Hmm. A bit of a problem there is that we do not necessarily have to run build method. What I really am after is to be able to get the updated metadata even if there is no build invoked (and that's why I moved versions to constructor). The problem is that BuilderInterface is created, but then it is queried about metadata (and update_metadata hook gets invoked) way before build method is called (and actually build method might not be called if the fronted just retrieves metadata without doing the build).

@potiuk
Copy link
Contributor Author

potiuk commented May 27, 2024

I can't remember the full execution path - but every time self.builder.metadata.core in BuilderConfig is accessed, the "update_metadata" hook is called (way before `for loop§ is entered.

@potiuk
Copy link
Contributor Author

potiuk commented May 28, 2024

Some more details @ofek - Even if build() method is called on BuildInterface - what happens here, that "metadata_hook" is called during validate_fields - so way before we enter the loop on versions. See the stack-trace below:

     Traceback (most recent call last):
        File "/Users/jarek/Library/Application Support/hatch/env/virtual/apache-airflow/MvrXOyBN/apache-airflow/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/Users/jarek/Library/Application Support/hatch/env/virtual/apache-airflow/MvrXOyBN/apache-airflow/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
        File "/Users/jarek/Library/Application Support/hatch/env/virtual/apache-airflow/MvrXOyBN/apache-airflow/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 176, in prepare_metadata_for_build_editable
          whl_basename = build_hook(metadata_directory, config_settings)
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/build.py", line 83, in build_editable
          return os.path.basename(next(builder.build(directory=wheel_directory)))
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/builders/plugin/interface.py", line 91, in build
          self.metadata.validate_fields()
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/metadata/core.py", line 277, in validate_fields
          _ = self.version
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/metadata/core.py", line 160, in version
          self._version = self._get_version()
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/metadata/core.py", line 256, in _get_version
          core_metadata = self.core
        File "/private/var/folders/v3/gvj4_mw152q556w2rrh7m46w0000gn/T/pip-build-env-rbopbuup/overlay/lib/python3.8/site-packages/hatchling/metadata/core.py", line 208, in core
          metadata_hook.update(self.core_raw_metadata)

By the time we get to the for loop you suggested, the update_metadata hook has already been called.

This is why I moved the versions up to ProjectMetadata and other places.

@potiuk
Copy link
Contributor Author

potiuk commented May 29, 2024

I have experimented a bit more with different approach: where update_metadata hook would be called several times - potentially - when version is retrieved during validate_fields() at the beginning of build and later during the for loop for versions, however that requires some small tricks - for example when we retrieve dynamic values, currently hatchling removes them from the list of dynamic variables as they are retrieved (which means that next time they are seen as non-dynamic and there are "must be dynamic" errors thrown). But I feel this is a bit of red-herring to chase, I think that update_metadata hook should only be called once for BuilderInterface object. I wonder if better idea will be to skip validation at build entry for dynamic values (but that would require to do the validation in a bit smarter way). Would love to hear what you think of that.

@ofek
Copy link
Collaborator

ofek commented Jun 1, 2024

Okay I thought about this and have a really good idea! I think this build indicator should be an environment variable that the caller sets. This would mean the Hatch CLI wouldn't have to change (unless you want a flag) and then inside the build method you would temporarily set the variable when calling the field validation method first thing. The environment variable could be called HATCH_BUILD_TARGET_<TARGET>.

@potiuk
Copy link
Contributor Author

potiuk commented Jun 1, 2024

Okay I thought about this and have a really good idea! I think this build indicator should be an environment variable that the caller sets. This would mean the Hatch CLI wouldn't have to change (unless you want a flag) and then inside the build method you would temporarily set the variable when calling the field validation method first thing. The environment variable could be called HATCH_BUILD_TARGET_.

Sounds good. Let me see where I can get itl

@ofek
Copy link
Collaborator

ofek commented Jun 1, 2024

Thanks! I know you know what I meant but I made a typo, the environment variable should be HATCH_BUILD_TARGET and set to the target name.

edit: I just checked and it doesn't conflict with existing configuration https://hatch.pypa.io/latest/config/build/#environment-variables

@potiuk
Copy link
Contributor Author

potiuk commented Jun 1, 2024

Yep. Understood :)

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.

Feature: Pass version to custom metadata hook
2 participants