Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Embed git commit hash into Python source #319

Merged
merged 15 commits into from
Jun 27, 2024

Conversation

dbarbuzzi
Copy link

@dbarbuzzi dbarbuzzi commented Jun 21, 2024

This PR performs a simple embed of the current git commit hash into the Python source:

  1. The hash itself is defined in the __githash__ __commit__ var in vllm/version.py alongside __version__
  2. vllm/__init__.py is updated to (a) import __githash__ __commit__ and (b) include it in the __all__ list

As a result, you can now do the following:

import vllm
print(vllm.__version__)  # already present
print(vllm.__commit__)  # now available

Test Plan
New test added which minimally verifies that this new variable is defined to a value with a bare-minimum length.

@dbarbuzzi dbarbuzzi self-assigned this Jun 21, 2024
Copy link

@derekk-nm derekk-nm left a comment

Choose a reason for hiding this comment

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

Thank you! Thank you! Thank you!

Copy link
Collaborator

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic left a comment

Choose a reason for hiding this comment

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

This looks good.

Can you add a test that this is working properly?

@mgoin
Copy link
Member

mgoin commented Jun 22, 2024

Could we register it as vllm.__commit__? I believe this is what Simon requested upstream

@dbarbuzzi
Copy link
Author

Could we register it as vllm.commit? I believe this is what Simon requested upstream

Done @mgoin

@dbarbuzzi
Copy link
Author

Can you add a test that this is working properly?

@robertgshaw2-neuralmagic I've added a basic test here (checks that the variable is defined): https://github.com/neuralmagic/nm-vllm/blob/simple-githash-embed/tests/test_embedded_commit.py

Let me know if you want something more in-depth.

@dbarbuzzi
Copy link
Author

One of the LM-EVALs failed (think it’s the same one that failed in a nightly, marlin non-determinism?):

FAILED tests/accuracy/test_lm_eval_correctness.py::test_lm_eval_correctness[TechxGenus/Meta-Llama-3-8B-Instruct-GPTQ] - assert False
 +  where False = <function isclose at 0x7ad222e75bf0>(0.688, 0.648, rtol=0.05)
 +    where <function isclose at 0x7ad222e75bf0> = numpy.isclose

@mgoin
Copy link
Member

mgoin commented Jun 25, 2024

I don't like the fact that we can't rely on vllm.__commit__ existing in the general case since this only happen during build. Could you at least add an empty __commit__ = None to version.py so the attribute is checkable?

This is also a bit specific to our build process. I thought we would integrate this more into the vllm build process natively. Something like this would work towards that: https://github.com/thuml/depyf/blob/3574bc0f12d9baad751354b19c432c822f8d321f/setup.py#L7-L16

@dbarbuzzi
Copy link
Author

@mgoin I’ve updated the PR to embed it during setup.py. All plumbing is added to the __init__.py and version.py files with a placeholder for the value in the latter file.

However, my understanding is that we want this in all builds (nightly, release, etc.), I diverged from that example slightly during setup.py and instead it still directly calls the function that does the embedding so that version.py is re-written with the placeholder replaced. The way the example has it, the commit would be part of the version/tag that is present in the metadata on e.g. PyPI/pip which, to my knowledge, is not the path we want (let me know if that’s incorrect).

Copy link
Member

@andy-neuma andy-neuma left a comment

Choose a reason for hiding this comment

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

just some minor nits.

.github/actions/nm-build-vllm/action.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
vllm/version.py Outdated Show resolved Hide resolved
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Looks perfect to me, thanks!

@@ -1,2 +1,3 @@
# UPSTREAM SYNC: take downstream
__version__ = "0.5.1"
__commit__ = "COMMIT_HASH_PLACEHOLDER"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I do think this is a bit long as a placeholder or dummy value

Copy link
Author

Choose a reason for hiding this comment

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

Any suggestions for an appropriately short one? Maybe COMMIT_PLACEHOLDER, or is that still long?

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic merged commit 7c66172 into main Jun 27, 2024
28 checks passed
@robertgshaw2-neuralmagic robertgshaw2-neuralmagic deleted the simple-githash-embed branch June 27, 2024 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants