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

Fix Benchmarks 2024 #1764

Merged
merged 7 commits into from Jan 18, 2024
Merged

Fix Benchmarks 2024 #1764

merged 7 commits into from Jan 18, 2024

Conversation

trisyoungs
Copy link
Member

@trisyoungs trisyoungs commented Jan 17, 2024

This PR addresses a couple of issues with the benchmarks.yml workflow. The initial error was for a missing header (<oneapi/dpl/iterator>) but this turned out to be a trivial fix - the "pure" Linux builds (of which the benchmarks workflow is one example) leverages conan, and our CMakeLists.txt wasn't explicitly passing on paths to the onedpl package.

I have also modified the workflow to always run, so we can benchmark PRs, but to only push benchmark results if on develop.

One small thing for general comment - it would be nice to have a single source of truth for the versions of the various packages we use. Nix and conan are independent in that sense right now, so thoughts on harmonising this are welcome.

Copy link
Contributor

@rhinoella rhinoella left a comment

Choose a reason for hiding this comment

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

This part is unneeded now

CMakeLists.txt Outdated Show resolved Hide resolved
@rprospero
Copy link
Contributor

@trisyoungs I agree about desiring a single source of truth for the version numbers. Unfortunately, it's less clear on how to get there. Here's the options that I've looked at:

  1. Have nix pull the version numbers from the conan file. This makes conanfile.txt the single source of truth. The major issue with this is that the versions available from conan often aren't the same as the versions available from nixpkgs. We can circumvent this by writing some nix shims to load those versions ourselves. This means we wind up manually building more (though it should be possible to cache this) and that we have to write all of these shims. The framework for most of this code would be found in the nixpkgs repo. Optimistically, I'd estimate that someone would need a week to put these shims in. Pessimistically, a month. These shims would also likely need to be updated when we update dependency versions.
  2. Have nix literally use conan. This make the conanfile.txt the single source of truth and slightly simplifies our CMake files. The disadvantage is that nix really does not want to do this. Nix builds aren't allowed to download anything from the internet, both to ensure reproducibility and improve caching. Conan obviously needs to download the sources from the internet. We can tell nix to work in an impure mode that allows conan to performs the downloads that it needs, but this breaks several assumptions that nix makes. It's possible to do it correctly, but there's a bunch of weird failure cases that this can fall into (e.g. silently continuing to use old versions after an atempted update, rebuilding dissolve three times whenever we produce an image).
  3. Have nix produce the conan file. This makes nix the single source of truth for the versions. Since the nix versions don't always align with the versions of conan center, we would need to host our own conan server. That's obviously a huge amount of administrative overhead, but it would at least also freeze the conan versions so they don't suddenly change and break the windows and mac builds, too.
  4. Have nix replace conan. This makes nix the single source of truth. Once we no longer need to patch qt versions, I might try this again for the Mac build. Unfortunately, that still leaves the Windows build requiring Conan versions. This would likely mean adding a good deal of scripting to the windows CI to manually download all of the libraries. On the plus side, this scriping could also be re-used by developers on the Windows platform for setting up their system for pure Windows builds (instead of a pseudo Linux build through Docker or IDAaaS).
  5. Have conan replace nix. This makes conan the single source of truth. This would require new scripting in the CI for producing distributable Linux builds and singularity images.
  6. Punt. We leave no single source of truth and continue to manually reconcile the version numbers when we update conan or nixpkgs. Over the course of a few years, this is likely the same as very slowly doing option 1.

@trisyoungs
Copy link
Member Author

Comprehensive, and totally correct, assessment @rprospero. Think you've convinced me that 6) is the best option with the least hassle!

Co-authored-by: Noella Spitz <101950441+rhinoella@users.noreply.github.com>
@rhinoella rhinoella self-requested a review January 18, 2024 09:42
@trisyoungs trisyoungs merged commit 10b86fa into develop Jan 18, 2024
8 checks passed
@trisyoungs trisyoungs deleted the fix-benchmarks-2024 branch January 18, 2024 10:03
trisyoungs added a commit that referenced this pull request Jan 26, 2024
Co-authored-by: Noella Spitz <101950441+rhinoella@users.noreply.github.com>
rprospero pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: Noella Spitz <101950441+rhinoella@users.noreply.github.com>
rprospero pushed a commit that referenced this pull request Apr 10, 2024
Co-authored-by: Noella Spitz <101950441+rhinoella@users.noreply.github.com>
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.

None yet

3 participants