-
Notifications
You must be signed in to change notification settings - Fork 16
switch hatch to dependency groups, split deps to data file #144
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
base: main
Are you sure you want to change the base?
switch hatch to dependency groups, split deps to data file #144
Conversation
… is not happy about this
|
My guess is the tests here are breaking not because of this pr but they have been broken. I am not sure why however as they started breaking when we got the latest dep update and I made changes in november but they were ok then. It is possible that I made something brittle. |
|
I think its just this! pypa/hatch#2152 |
|
@sneakers-the-rat taking a brief look through this, I'm not convinced that defining dependencies in a separate YAML file provides real value. To me it seems to introduce more complexity to the template and separates the definition of the groups from where they are used. Basically, I don't see the disadvantage of having the constraints in the template for the |
|
maintainable templates separate data from structure with some reasonable balance of which things might be changed or be reused. so now rather than the dependencies section having the logical conditions that control rendering interspersed with data, we have a compact section of logical conditions without needing to repeat the formatting that is shared between all the things of the same kind. the circumstance where it is a useful change is if we ever want to do any logic on the deps - e.g., maybe allow someone to use extras instead of dependency-groups for some subset of the groups (i would like that for shipping tests in the package), or declare additional dependencies in some tool-specific way like in hatch build env dependencies, allow someone to enable/disable using the minimum constraints, provide their own default constraints, update their generated project from specifically, the reason why i had this in mind for awhile was thinking about how it would be a bunch of continuous annoying work keep any minimums updated e.g. for security problems from audit, so having them as data lets them use them as data - can write an action to check all the packages used in the template, across all options that might change, with the specific version dependency, and then autogenerate a PR that bumps the version. doing that by parsing all the deps as text in the template would be a pain to say the least. the implementation as is should only ever need to be changed if toml, copier, or jinja changed core features, and at that point we'd need to probably refactor things anyway. idrc, close this if you want, not a change i'd fight for, if it's controversial i'd rather just close it and forget about it edit: the reason why i thought this made sense for the deps specifically and not other things is that they are the most likely thing to want to programmatically control or introspect. like i am not trying to put all strings in separate files, some things make sense to do in place, etc. |
template/pyproject.toml.jinja
Outdated
| # Use UV to create Hatch environments | ||
| [tool.hatch.envs.default] | ||
| installer = "uv" | ||
| builder = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, in theory, should work, but I found it doesn't.
this pr - pypa/hatch#2155 is working towards. fix. so i think we should add builder=True here to the individual environments. and then open an issue to remove that when they have fixed the bug!
| COPIER_CONFIG_PATH = Path(__file__).parents[1] / "copier.yml" | ||
| INCLUDES_PATH = Path(__file__).parents[1] / "includes" | ||
|
|
||
| # handle copier's !include tags - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, OK. So we are modifying this function rather than using what the copier uses, to allow us to import the dependency files effectively in a way that we can trust the "API" part won't change on us? Is that the gist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's basically so that we do effectively what they do in the tests where we are loading the copier template directly (rather than calling copier, which we do in most of the tests) without reaching inside their private methods. also they use pyyaml and we are using ruamel.yaml, so had to adapt it for that too
lwasser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneakers-the-rat, see if my suggested changes fix CI? I haven't pulled this down to test.
Given they are actively working on a fix to this bug, I think it's easy enough to switch things over, use builder = True, and then remove it once hte patch is released. This way we are following the modern pep convention of using dependency groups but using a quirky work-around (a hatch feature that we don't need for pure python) that we can easily and simply remove once hatch supports it properly.
|
@sneakers-the-rat I have a slight concern about increase in complexity and decrease in cohesion, but your arguments make sense even if we don't make use of these new facilities immediately. And importantly, if this brings you joy, I have no wish to stand in the way or even ask to abandon the work. I'll follow with some inline comments. |
| {%- if not deps[0] is mapping %} | ||
| {%- set d=deps[0][0] %} | ||
| {%- else -%} | ||
| {%- set d=deps[0] %} | ||
| {%- endif -%} | ||
| {%- if group and group in d -%} | ||
| {%- if not inner -%} | ||
| {{ group if "_rename" not in d[group] else d[group]["_rename"] }} = [ | ||
| {%- endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do away with most of these checks. Instead of extracting the group from the global deps variable, rather expect the group parameter to already be an appropriate mapping. So I'd call it with something like render_deps(deps["tests"]). And I'd favor calling it render_dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to get copier to render it as an appropriate mapping pretty much exactly like this, but copier insists on loading the yaml as a length one array for some reason, and it seems to vary by version where older python double nests it. This was the one point where i almost gave up and would say would be reason not to make the change if we were to pick one, but i figured if this was the only way to provide data to it as one can do it most other templating systems, this would be the workaround needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah its not great, i figured there would be some way of e.g. declaring a directory of data to load into the jinja environment, but the only way i found to do it really well was to do this: https://copier.readthedocs.io/en/stable/faq/#how-can-i-alter-the-context-before-rendering-the-project
But that makes the template "unsafe" which obviously is too large of a downgrade to warrant it.
| twine: | ||
|
|
||
| dev: | ||
| hatch: ">=1.16.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had the discussion on Slack regarding a lack of being able to require a minimum hatch version, despite that, we need hatch as a global dependency to even create the hatch environments, so this dependency does not make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that it would be able to detect a mismatch of version between the installed hatch and the requested hatch when it created the build env it used to install the package, but that might have been an empty hope. Im not sure where this dep should get declared but we need to declare it somewhere, because now the template wouldnt work with hatch versions older than this. So i guess this is a problem we would need to fix for whatever implementation of dependency-groups we do here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we think about the hatch dependency in the same way we think about python. This template supports hatch >= version x.x.x in our documentation? I know it's not a perfect solution.
The better solution would be something akin to python-requires
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think in any case it's sort of at a different level than the template itself, it seems like a feature that hatch needs to me. the thing i worry about with putting dependencies in the README is that it's much easier to not see them than programmatic specifications that force them to be true, but I added a note in the readme here: b1dd00d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely not a concern for your pr here! agree 💯 !!
| {%- if use_test %}{{ "\n" }}{{ render_deps("tests") }}{% endif %} | ||
| {%- if use_lint %}{{ "\n" }}{{ render_deps("style") }}{% endif %} | ||
| {%- if use_types %}{{ "\n" }}{{ render_deps("types") }}{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always have to look up the whitespace behavior of jinja2, but isn't it possible to have each group start on a newline without inserting a literal \n?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also consider render_deps having only the inner=True behavior, that would likely solve the whitespace issues here. As you'd have to specify something like:
{%- if use_test %}
tests = [
{{ render_deps("tests") }}
]
{%- endif %}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way mostly because i wanted the top level pyproject.toml to be a logical template (what to display) and the macro to be the structural template (how to display it), and to link the keys in the dict with the output to avoid key discrepancies (tests/test). Its a little awkward but this way of writing them makes them self contained lines that dont have different start/end whitespace behavior that would need to be handled by, e.g., remembering to suppress whitespace at the end of the last line and start of first, and future groups could copy/paste the one line and just modify the key
Co-authored-by: Leah Wasser <leah@pyopensci.org>
|
looking at #145, and did this: 6c7bafb and apparently now the thing that makes the dependency groups work is to not have builder in the environments? and then the ones that do have however in copying the change from |
| dependency-groups = [ | ||
| "build", | ||
| ] | ||
| {{ render_deps("build", key="dependencies") }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this, btw, is why it's nice to have the dependencies separated out as data - if we need to do weird things with them like render them twice, we can do it without needing to remember to keep multiple places updated, all the deps stay specified in the yaml
|
@sneakers-the-rat i am testing this too... what do you think about separating concerns: move the dependencies to data files to a separate issue and pr so we can focus this one here only on supporting dependency groups? I am getting some inconsistent results ? Are you seeing that too? And what version of hatch are you testing on? i am testing on the new dev version. The ci here is i think working on the latest december release. I can run the docs environment without also hatch env remove doesn't actually remove the envt as far as i can see. I am happy to trouble shoot more next week when i'm back online. But i'm not convinced that the fix on their dev branch right now is actually working either. |
|
i posted here in hopes that will help. i also noticed that if you try hatch env remove it doesn't actually delete the environment. i have some theories around that (space in my path maybe) but there is something really odd happening with hatch. |
sure, the change to dependency groups is so small it doesn't really matter either way to me. the macro makes it easier to do the change if we need to render duplicate dependency groups in the hatch envs like the current workaround, but idrc. i'm using the latest pypi release (1.16.2) locally |
fix: #143
so this does two things, switched us over to dependency groups and splits our dependencies out into data so that the version constraints aren't all mushed up with the form of the template (which is about structure not data).
however for the life of me i can't tell what's going on - the
pyproject.tomlseems to be correctly generated, here is an example with theminimaloption:expand/collapse pyproject.toml
but when we go to actually run the functional tests, hatch doesn't seem to actually be installing any of the dependency group deps, or the package itself... I am pretty positive this has nothing to do with the templating change, when i diff the generated
pyproject.tomlbefore and after this PR, the only thing that changes pretty much is renaming the table key todependency-groups. I paused the pytest run on errors and went to the generated directory and it also looked fine, and same result - when i tried to run hatch manually it didn't install any of the deps and then errored out.I have been up and down hatch's documentation and i can't figure out why it doesn't seem to want to install anything!
the only other thing i can think of is it being related to pypa/hatch#2113 which seems to require that the
builder=trueflag be present for pretty much all the envs.i lay this PR on the mercy of the court, if anyone can figure this out, gr8
I am opening this up here to see if CI behaves differently and something is just wrong on my machine. idk.
edit: there does seem to be something going wrong where the outer hatch environment that’s running the tests for the template seems to interfere with the inner hatch environment that’s being tested - when i removed the test matrix from the template, another test failed saying that the python 3.10 environment didn’t exist, and it started working again when i removed the matrix from the outer (template) package…
i can also replicate the problems here by removing all the template changes except strictly changing the keys to
dependency-group, i think something broken in hatch.(call me old fashioned, but i am sorta liking my old fashioned ways of "simply using venvs" right now, at least with a venv i know exactly where it is and what's in it and there's no tool in between me and python, but all these environment virtualization tools seem to want to get too fancy and break lol)
edit2: ope wait no it's just this: pypa/hatch#2152