Add workflow_settings.free_disk_space#2568
Conversation
Add a new `workflow_settings.free_disk_space` setting as a replacement for the legacy `azure.free_disk_space` and `github_actions.free_disk_space`. The old values are converted automatically. All values are also normalized to arrays to simplify templates. The values can be conditional to individual jobs. Signed-off-by: Michał Górny <mgorny@quansight.com>
| ] = Field( | ||
| default=[], | ||
| description=cleandoc(""" | ||
| Free up disk space before running the Docker container for building on Linux. |
There was a problem hiding this comment.
Is it Linux only? 🤔 I thought we were going to do it OS-agnostic too.
There was a problem hiding this comment.
I suppose extending to other systems could be handled as a separate PR.
There was a problem hiding this comment.
I can live with deferring other platforms to a separate PR, but then we need some thought how os-specific workflow_settings will work. IMO there should be an error in smithy if someone does
workflow_settings:
free_disk_space: true
because it gives the false impression of being cross-platform.
build_workspace_dir already has a platform-specific warning (e.g. for setting windows paths on unix), so hopefully this should be a ~solved problem already.
There was a problem hiding this comment.
Okay, I'll start looking into other platforms.
There was a problem hiding this comment.
Ok, added macOS and Windows; with Windows supporting only cache and macOS not doing anything right now. Does anyone have a suggestion what we should be cleaning there?
There was a problem hiding this comment.
The description here needs an update, in one way or another
There was a problem hiding this comment.
Proposed something in https://github.com/conda-forge/conda-smithy/pull/2568/files#r3361885388
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
We've used `rsync` to workaround historical `rm` inefficiency but that's no longer the case. From a quick testing, both commands involve roughly the same syscalls, and there is no noticeable difference on GHA. Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
|
Demo feedstock: conda-forge/polars-feedstock#380 |
|
I can't reproduce the test failure locally. I suspect the schema generation is not reproducible. |
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
|
If you squint a bit, the following could also be considered conda-smithy/conda_smithy/templates/github-actions.yml.tmpl Lines 182 to 186 in 7ed6300 Not sure if/how we want to unify these things, but the named-arg-style approach already used linux might also be applicable to windows? |
|
Yeah, I suppose that'd work, and it would definitely make sense to avoid inlining that script. |
Signed-off-by: Michał Górny <mgorny@quansight.com>
To be honest, I'd keep it as a separate key. It's not like keys are expensive, and the backwards compatibility logic would end up being a bit messy here. And in the end, workflow_settings:
free_disk_space:
- os: win
value: resize-partitions
- os: linux
value: true |
|
Either way, I'd like to avoid adding more into this PR. We can add new functions as a followup, but I'd like to get what we have now reviewed. |
h-vetinari
left a comment
h-vetinari
left a comment
There was a problem hiding this comment.
Thinking about the interface here, and how to deal with platform-specific stuff in an ostensibly cross-platform value:
| bool, | ||
| list[Literal["apt", "cache", "docker"]], | ||
| list[ | ||
| conditional_value( | ||
| Union[bool, list[Literal["apt", "cache", "docker"]]], False | ||
| ) | ||
| ], |
There was a problem hiding this comment.
I'm unsure about the multi-type approach here, and mixing in platform-specific options. I think it may be cleaner/simpler to either do
free_disk_space: Optional[Union[Literal[None, "quick", "extensive"]]](or however the bikeshed is coloured), which would trade granularity for an easier interface1; or alternatively, we could make free_disk_space: Optional[bool] to just mean the cache portion, and introduce separate (but linux-specific-by-definition) switches for cleaning up docker & apt.
Footnotes
-
because, let's face it, for recipe authors it's a simple trade-off of "make my recipe pass" vs. "how much extra time does my CI take" ↩
There was a problem hiding this comment.
I mean, it could be both:
Union[
bool, # true -> quick
Literal["all"], # opt-in to everything
list[Literal["apt", "cache", "docker"]], # as granular as it gets
]There was a problem hiding this comment.
What I'm not super keen on is implying that apt or docker are legitimate values anywhere but on linux. But perhaps I'm over-designing this? Could be that it's not that important.
My comment is based on the fact that I've only ever seen true or the full set of apt, cache, docker for this, so I'm wondering if this granularity really buys us anything? I kinda like quick vs. extensive (or all), that should IMO provide enough for what recipe authors use this for.
BTW, I've used this PR today in conda-forge/pyg-lib-feedstock@6274e01, and I think that
workflow_settings:
free_disk_space:
- value: ["apt", "cache", "docker"]
os: linuxlooks weird? I mean having value: (itself already a list item) be again a list itself.
There was a problem hiding this comment.
If we're changing the format, then perhaps let's avoid lists indeed (it would also make the code cleaner). I'm fine with strings or string/bool, or even CSV for granularity.
One thing to consider is backwards compatibility, but I don't think there would be any harm even if we said that, say, apt alone is not supported anymore and either fall to not cleaning at all or cleaning more.
There was a problem hiding this comment.
Well, how about either:
- if we want real booleans,
false / true / "all" - if we just strings for fewer types:
"no" / "quick" / "all"
?
There was a problem hiding this comment.
"no" / "quick" / "all"
I like this 👍
There was a problem hiding this comment.
I'd maybe change "all" to "max". We're not deleting all things in the image after all, just the most we can. And "max" describes that more accurately, both in the sense of what can be deleted, as well as what we have actually implemented (or deemed worth implementing).
| ] = Field( | ||
| default=[], | ||
| description=cleandoc(""" | ||
| Free up disk space before running the Docker container for building on Linux. |
There was a problem hiding this comment.
The description here needs an update, in one way or another
h-vetinari
force-pushed
the
workflow-settings-free-disk-space
branch
from
2a52cd4 to
d5c333f
Compare
| Free up disk space before running the Docker container for building on Linux. | ||
| The following components can be cleaned up: `apt`, `cache`, `docker`. | ||
| When set to `true`, only `apt` and `cache` are cleaned up. | ||
| Set it to the full list to clean up all components. |
There was a problem hiding this comment.
My suggested description:
| Free up disk space before running the Docker container for building on Linux. | |
| The following components can be cleaned up: `apt`, `cache`, `docker`. | |
| When set to `true`, only `apt` and `cache` are cleaned up. | |
| Set it to the full list to clean up all components. | |
| Free up disk space before running the builds. | |
| - On Linux, the following components can be cleaned up: | |
| `apt`, `cache`, `docker`. When set to `true`, only `apt` | |
| and `cache` are used. Set it to the full list to | |
| clean up all components. | |
| - On macOS, only `cache` is supported (same as `true`). | |
| - On Windows, only `cache` is supported (same as `true`). | |
| Note that only `C:/` is cleaned up, so this is only useful | |
| if you are targeting this drive via `build_workspace_dir`, | |
| `tools_install_dir`, or `pagefile_size`. |
There was a problem hiding this comment.
Uh, sorry, changed it prior to getting down to your comment. However, let's discuss the actual values first perhaps :-).
There was a problem hiding this comment.
Jaime 👍'd your comment about changing away from list-as-value. So it appears that the next step would be to make a proposal how that would look like.
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
3 participants
Checklist
newsentry with any new deprecations added to theDeprecatedsection.python -m conda_smithy.schema)python -m conda_smithy.linter.messages)Part of issue #2349
Fixes #1949
Closes #1966
Add a new
workflow_settings.free_disk_spacesetting as a replacement for the legacyazure.free_disk_spaceandgithub_actions.free_disk_space. The old values are converted automatically. All values are also normalized to arrays to simplify templates. The values can be conditional to individual jobs.Progress: