Skip to content
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

Add switch to make a swap file #1751

Merged
merged 17 commits into from
Aug 29, 2023

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented Jul 25, 2023

I found that while working on conda-forge/staged-recipes#22691 I needed to create a swap file for the build to complete since it exhausted the host memory.

Checklist

  • Added a news entry

@mikemhenry
Copy link
Contributor Author

Also even with this on, when used with the free disk space switch, there is a net gain in disk space.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Mike! 🙏

Wonder if we could customize the size here

conda_smithy/templates/azure-pipelines-linux.yml.tmpl Outdated Show resolved Hide resolved
conda_smithy/templates/azure-pipelines-linux.yml.tmpl Outdated Show resolved Hide resolved
conda_smithy/templates/azure-pipelines-linux.yml.tmpl Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>

**Changed:**

* Create toggle to make a 10G swap file on the default linux image on Azure Pipelines
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to update this

@mikemhenry
Copy link
Contributor Author

Looks like it accepts these units:

Arguments:
 <num> arguments may be followed by the suffixes for
   GiB, TiB, PiB, EiB, ZiB, and YiB (the "iB" is optional)

I do like the idea of letting users just set it, I will add a note in the docs on how to use it.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Would also add a default like "0GiB" in this config section

conda_smithy/templates/azure-pipelines-linux.yml.tmpl Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@mikemhenry
Copy link
Contributor Author

Sorry for all the weird suggestions, I was on my phone 🙃

@jakirkham
Copy link
Member

No worries.

Could you please also add a default like "0GiB" for this parameter in the default configs:

@mikemhenry
Copy link
Contributor Author

I think I did that correctly

@jakirkham
Copy link
Member

Thoughts @conda-forge/core ?

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I'm confused. Given the current logic a swapfile would always be made. Don't we want it to not run unless the size is great than zero?

@h-vetinari
Copy link
Member

If we're making a new switch for swap files, it would probably make sense to tie this in with conda-forge/conda-forge-ci-setup-feedstock#155 / conda-forge/conda-forge-ci-setup-feedstock#158, which is currently activated by:

azure:
  settings_win:
    variables:
      SET_PAGEFILE: "True"

I had an unmerged PR that would have allowed setting the size too, if we want to match it in capabilities with linux.

Not sure about where the PowerShell script would have to live, but IIUC, conda-smithy would just generate the call in azure-pipelines-wind.yml.tmpl, but could rely on the fact that the respective PowerShell script is present after conda-forge-ci-setup has been installed.

Co-authored-by: jakirkham <jakirkham@gmail.com>
@beckermr
Copy link
Member

@h-vetinari do you think we should combine this with that other work now or is this more of something to do in the future?

mikemhenry added a commit to mikemhenry/dgl-feedstock that referenced this pull request Aug 16, 2023
@mikemhenry
Copy link
Contributor Author

Also re testing this:

Here is a commit after I ran the rendering without setting the size of the file conda-forge/dgl-feedstock@25c6e87

And here I set the size conda-forge/dgl-feedstock@0a5b406

And here we have the result conda-forge/dgl-feedstock@edeb18f

@jakirkham
Copy link
Member

Thanks Mike! 🙏

Sorry for asking this after the work above (thought I had already written this question)

Is it possible that we might want different swap file sizes on Linux & Windows? If so, would there be value in having these be different config values?

Should add these questions are for everyone on this thread (or outside of it that have interest)

@mikemhenry
Copy link
Contributor Author

Is it possible that we might want different swap file sizes on Linux & Windows? If so, would there be value in having these be different config values?

That is a good point, I was trying to lean into the comment here #1751 (comment) for it to be a single api entry point (if you want to call it that).

Personally, I think a azure.linux_settings.swapfile_size and a azure.win_settings.swapfile_size both make sense, and then could be independent. (those names might not be exactly right, I didn't lookup what we have in the yaml)

It might be nice for them to be separate since it would be silly to get into a situation where you need swap on Linux, but more disk space on Windows, and are kinda stuck with only one knob to turn.

@pavelzw
Copy link
Member

pavelzw commented Aug 18, 2023

Thanks a lot for this PR @mikemhenry! We tried it out in conda-forge/polars-feedstock#161 and it works great 🤩 hope this gets merged soon :)

@mikemhenry
Copy link
Contributor Author

Latest test here: conda-forge/dgl-feedstock@42e53d9

@mikemhenry
Copy link
Contributor Author

This is working for me, I am not sure why CI is failing now:

_________________________ test_upload_on_branch_azure __________________________
Traceback (most recent call last):
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/runner.py", line 341, in from_call
    result: Optional[TResult] = func()
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/runner.py", line 262, in <lambda>
    lambda: ihook(item=item, **kwds), when=when, reraise=reraise
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_hooks.py", line 433, in __call__
    return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_manager.py", line 112, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_callers.py", line 155, in _multicall
    return outcome.get_result()
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_result.py", line 108, in get_result
    raise exc.with_traceback(exc.__traceback__)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_callers.py", line 80, in _multicall
    res = hook_impl.function(*args)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/runner.py", line 177, in pytest_runtest_call
    raise e
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/runner.py", line 169, in pytest_runtest_call
    item.runtest()
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/python.py", line 1788, in runtest
    self.ihook.pytest_pyfunc_call(pyfuncitem=self)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_hooks.py", line 433, in __call__
    return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_manager.py", line 112, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_callers.py", line 116, in _multicall
    raise exception.with_traceback(exception.__traceback__)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/pluggy/_callers.py", line 80, in _multicall
    res = hook_impl.function(*args)
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/_pytest/python.py", line 194, in pytest_pyfunc_call
    result = testfunction(**testargs)
  File "/home/runner/work/conda-smithy/conda-smithy/tests/test_configure_feedstock.py", line 263, in test_upload_on_branch_azure
    assert (
AssertionError: assert 'UPLOAD_ON_BRANCH="foo-branch"' in 'docker run --rm --privileged multiarch/qemu-user-static:register --reset --credential yes\nls /proc/sys/fs/binfmt_misc/\n'

@mikemhenry
Copy link
Contributor Author

@jakirkham any ideas?

@jakirkham
Copy link
Member

Maybe it was a fluke? Restarting CI

Co-authored-by: jakirkham <jakirkham@gmail.com>
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Mike and everyone for the reviews! 🙏

@mikemhenry
Copy link
Contributor Author

Doc entry is here conda-forge/conda-forge.github.io#1988

@jakirkham
Copy link
Member

Any last thoughts on this addition?

@jakirkham jakirkham merged commit dae12a2 into conda-forge:main Aug 29, 2023
2 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

If anything else comes up, please raise an issue and we can follow up there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants