hatch icon indicating copy to clipboard operation
hatch copied to clipboard

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

Open potiuk opened this issue 1 year ago • 14 comments

This is a very draft proposal on how to fix https://github.com/pypa/hatch/issues/1348 and https://github.com/pypa/hatch/issues/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 avatar May 20 '24 21:05 potiuk

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

potiuk avatar May 20 '24 21:05 potiuk

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

ofek avatar May 23 '24 13:05 ofek

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 avatar May 27 '24 11:05 potiuk

Smth like that ?

potiuk avatar May 27 '24 21:05 potiuk

Or did you think to bubble it up to BuilderInterface ?

potiuk avatar May 27 '24 21:05 potiuk

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.

ofek avatar May 27 '24 21:05 ofek

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 avatar May 27 '24 21:05 potiuk

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 avatar May 27 '24 21:05 potiuk

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 avatar May 28 '24 19:05 potiuk

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.

potiuk avatar May 29 '24 18:05 potiuk

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>.

ofek avatar Jun 01 '24 00:06 ofek

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>.

Sounds good. Let me see where I can get itl

potiuk avatar Jun 01 '24 10:06 potiuk

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

ofek avatar Jun 01 '24 13:06 ofek

Yep. Understood :)

potiuk avatar Jun 01 '24 16:06 potiuk