Skip to content

Conversation

@FBruzzesi
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

@FBruzzesi FBruzzesi changed the title Chore/less version manual parsing chore: Bump version manually only in pyproject.toml May 8, 2025
@FBruzzesi
Copy link
Member Author

FBruzzesi commented May 8, 2025

I can definitly replicate the issue locally, but I hoped that it was some env issue πŸ₯²
It looks like that pre-commit does not install narwhals when running check-api-reference πŸ€”

Edit: changing the following:

- entry: python -m utils.check_api_reference
+ entry: python utils/check_api_reference.py

leads to

ModuleNotFoundError: No module named 'narwhals'

so yes, narwhals is not installed πŸ€”

@dangotbanned
Copy link
Member

@FBruzzesi I wonder if the new uv version command might be helpful for us here?

@FBruzzesi
Copy link
Member Author

@FBruzzesi I wonder if the new uv version command might be helpful for us here?

@dangotbanned this PR was aiming at something else, but we could definitly replace:

with open("pyproject.toml", encoding="utf-8") as f:
content = f.read()
old_version = re.search(r'version = "(.*)"', content).group(1) # pyright: ignore[reportOptionalMemberAccess]
version = old_version.split(".")
if how == "patch":
version = ".".join(version[:-1] + [str(int(version[-1]) + 1)])
elif how == "minor":
version = ".".join(version[:-2] + [str(int(version[-2]) + 1), "0"])
elif how == "major":
version = ".".join([str(int(version[0]) + 1), "0", "0"])
content = content.replace(f'version = "{old_version}"', f'version = "{version}"')
with open("pyproject.toml", "w", encoding="utf-8") as f:
f.write(content)

with

subprocess.run(["uv", "version", "--bump", how])

🀩

@FBruzzesi FBruzzesi marked this pull request as ready for review May 8, 2025 17:09
@FBruzzesi FBruzzesi marked this pull request as draft May 8, 2025 17:11
@dangotbanned dangotbanned added ci and removed internal labels May 8, 2025
@FBruzzesi FBruzzesi marked this pull request as ready for review May 9, 2025 19:52
Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

Thanks @FBruzzesi πŸ₯³

]


def __getattr__(name: _t.Literal["__version__"]) -> str: # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

To be completely honest I'd rather just update in two places, I have no idea what this is doing

Happy to take the installation update in the docs page though πŸ‘

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make sure I understand correctly - the concern with importing importlib.metadata is regarding startup time? see comment

Copy link
Member

Choose a reason for hiding this comment

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

i feel uneasy about the global - do you have a reference from another project that does this?

Copy link
Member Author

@FBruzzesi FBruzzesi May 10, 2025

Choose a reason for hiding this comment

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

Looking for __version__ = version(__name__) on github you will get many results

A couple of projects I know that made it in the first few pages of the list are:

Edit: some other project do the analogous of __version__ = version("narwhals") (but couldn't find something I know)

Copy link
Member

Choose a reason for hiding this comment

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

sure, thanks

still not sure we need it though - there's some other nice improvements in here though, happy to take those

regarding perf, from narwhals import __version__ currently takes 0m0.156s, and from importlib import metadata takes 0m0.108s

Copy link
Member

@dangotbanned dangotbanned May 10, 2025

Choose a reason for hiding this comment

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

Some notes:

IMO, I don't think there's a use-case for checking narwhals.__version__, but if people want to do that - the cost of it isn't paid by everyone else since we're providing it lazily.
Are there any search results of a package checking it? (sorry I'm replying from my phone πŸ˜…)

@MarcoGorelli I understand your caution and ultimately will defer πŸ˜‰ to you, but I think this balances:

  • Theoretical backwards-compatibility
  • Avoids introducing performance overhead for those that don't need it
  • Ensures that potential cost is only paid once
  • Lowers our maintenance burden, by simplifying version bumps

Copy link
Member

@MarcoGorelli MarcoGorelli May 10, 2025

Choose a reason for hiding this comment

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

thanks - could you elaborate on the global part please? i've had a look at the linked projects and they don't seem to have it

if we don't deem __version__ to be useful to most users, presumably we don't need the global either?

Copy link
Member

@dangotbanned dangotbanned May 10, 2025

Choose a reason for hiding this comment

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

thanks - could you elaborate on the global part please? i've had a look at the linked projects and they don't seem to have it

Sure @MarcoGorelli!

I'm assuming you're referring to the projects in (#2514 (comment))?
The 3 stdlib examples I gave in (#2514 (comment)) all featured use of global:

I guess for some more info on global, these might be helpful

As a more concrete example, we can do the following on main:

import narwhals as nw

>>> nw.__version__
'1.38.2'
>>> "__version__" in nw.__dict__
True
>>> nw.__dict__["__version__"]
'1.38.2'
>>> nw.__version__ == nw.__dict__["__version__"] == nw.__dict__.__getitem__("__version__")
True

But in this PR, we can see where the __getattr__ hook slips in:

import narwhals as nw

>>> nw.__dict__["__version__"]
KeyError: '__version__'
>>> nw.__version__
'1.38.2'
>>> nw.__dict__["__version__"]
'1.38.2'

Without the global:

  • __version__ is never added to nw.__dict__
  • Each lookup of nw.__version__ always goes via __getattr__, which is triggered after the regular __dict__ lookup fails

So the global is what provides the lazy import, since the work is only ever done once and only when a user asks for it πŸ™‚.
All subsequent lookups behave identically to a regular lookup into the namespace.


As another example, polars has 2 behaviors related to __getattr__:

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

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for the detailed explanations and for bearing with me on this one, really appreciate it

thanks both!

@MarcoGorelli MarcoGorelli merged commit b7001e4 into main May 11, 2025
31 of 32 checks passed
@MarcoGorelli MarcoGorelli deleted the chore/less-version-manual-parsing branch May 11, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants