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

split outputs, build with rattler-build #98

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

minrk
Copy link
Member

@minrk minrk commented Nov 25, 2024

use rattler because this is a lot of outputs and it saves a lot of time for the many env setup/teardown transactions over conda-build

adds suitesparse-mongoose as an additional component and unskips spex on windows, now that the required gmp is available there.

Further splitting not done, that could be, and I think could be done later without major consequence:

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Nov 25, 2024

Hi! This is the friendly automated conda-forge-linting service.

I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint --conda-forge . from the recipe directory. You can also examine the workflow logs for more detail.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12031395698. Examine the logs at this URL for more detail.

use rattler because this is a lot of outputs and it takes forever with conda-build

adds mongoose as an additional component
@minrk
Copy link
Member Author

minrk commented Nov 26, 2024

lint error is conda-forge/conda-smithy#2165 already fixed by conda-forge/conda-smithy#2166

@minrk
Copy link
Member Author

minrk commented Nov 26, 2024

This actually runs all the builds independently in sequence. A faster approach would be to keep the single 'build and install everything' and use files. Downside of files: we need to keep track of which files go with which package, and know about every added file for each release. The approach in this PR is a lot easier to keep correct, at the expense of some time (still only 5 minutes on linux-64 thanks to the switch to rattler-build).

@xhochy
Copy link
Member

xhochy commented Nov 26, 2024

At this point you could also think of splitting it into libpkg and libpkg-devel. If you do it at a later point, you can no longer use libpkg as the package containing only the runtime library as this would probably break existing packages (too much).

@minrk
Copy link
Member Author

minrk commented Nov 26, 2024

@xhochy that's a good point. I did think of that and figured the cost of the dev files wasn't big enough to justify the recipe complexity. It's a little hard with this multi-package layout where multiple builds have to run unless I go with a single build and pure files splitting since I don't think I can run a bunch of builds and do files splitting from several different build steps.

One way to do the splitting that I think would be maintainable is to switch to a single build and do files splitting where:

  • lib* packages include only libs (a simple glob for each)
  • a single suitesparse-devel includes all the headers/cmake files, making the file-include patterns tractable (include/, lib/cmake/ etc.)

so that a package depending on e.g. umfpack would have host:

- libumfpack # lib, run_exports
- suitesparse-devel # headers, etc., no run_exports

Do you think that's worth it?

Some numbers for the fully installed suitesparse to help inform the choice, which should be unchanged (plus the addition of suitesparse-mongoose):

  • total instal size: 5.4MB, 159 files
  • libs: 4.1MB, 51 files (2/3 are symlinks)
  • include: 800kB, 19 files
  • cmake, pkg-config: 500kB, 89 files

so the devel package would reduce the number of files for a full suitesparse install by 2/3 and the size by only 20%.

The single-build would also have to wait for conda-forge/conda-smithy#2057 to resolve because rattler-build currently has the build cache behind an --experimental flag we can't yet pass.

@xhochy
Copy link
Member

xhochy commented Nov 26, 2024

@xhochy that's a good point. I did think of that and figured the cost of the dev files wasn't big enough to justify the recipe complexity.

That is something where I don't have an opinion on as I am not a user of this package, but I stumbled on this case of libpkg vs libpkg-devel several times in the last days and wanted to raise it again, so that if you would have long-term plans for it, that you don't "fall into the same trap":

@minrk
Copy link
Member Author

minrk commented Nov 26, 2024

Since most packages don't split -devel on conda-forge, I've mostly been doing it considering the size/quantity of devel files compared to both the library itself, and the size/frequency of a typical env that depends on it. 1MB of headers in an env that's unlikely to be less that 300MB due to other dependencies doesn't seem entirely worth it.

I also don't think splitting further libumfpack into libumfpack-devel down the line is likely to break anything, as it would only mean the next build depending on libumfpack would need to add -devel, but runtime would only see an install-size savings without any breakage. Or is there something I'm missing there?

required gmp is now available, so give it a go
gives slightly clearer if/else for single dependency category (cmake generator)
@minrk minrk marked this pull request as ready for review November 26, 2024 11:39
@minrk
Copy link
Member Author

minrk commented Nov 26, 2024

@conda-forge-admin please lint

recipe/recipe.yaml Outdated Show resolved Hide resolved
- ${{ compiler('c') }}
- ${{ stdlib('c') }}
- cmake
- {if: win, then: ninja, else: make}
Copy link
Member

Choose a reason for hiding this comment

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

nice 👀

Copy link
Member

Choose a reason for hiding this comment

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

I am sure you know, but inline jinja should also work (${{ "ninja" if win else "make" }})

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the vague feeling that using logic expressed in static yaml would be a little less fiddly for tooling like conda-smithy than using a single string template. Do you think that's true? I'm sure it doesn't really matter for this particular dependency, but I'd love to know more about best practices.

Copy link
Member

Choose a reason for hiding this comment

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

For rattler-build it doesn't really matter as we have to parse the jinja AST in both cases anyways (the if: <cond> is also evaluated with minijinja, and we extract any used variables from there before rendering the recipe).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had the model in mind that when conda-smithy might be collecting the list of dependencies, it can concatenate all the lists and ignore the conditionals, assuming every branch will be taken by some build. But that may not have anything to do with what actually happens. And presumably it still needs to evaluate jinja for every build matrix entry anyway for completeness, so it probably doesn't make a difference.

I just get the feeling that it's nice to avoid templates where possible in the new format, even if it doesn't actually matter in practice.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/recipe.yaml) and found it was in an excellent condition.

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.

4 participants