Skip to content

Conversation

@tcojean
Copy link

@tcojean tcojean commented Feb 26, 2025

This facilitates contribution, keeping the repositories in sync, and expands from simple examples.

Cleanup is necessary to make this more convenient for CI and more consistent.

Terry Cojean and others added 2 commits February 26, 2025 23:24
This facilitates contribution, keeping the repositories in sync, and exapnds from simple examples.
Cleanup is necessary to make this more convenient for CI and more consistent.

Co-authored-by: Max Melnichenko <m.y.melnich@gmail.com>
@tcojean tcojean self-assigned this Mar 3, 2025
@BenBrock
Copy link
Collaborator

BenBrock commented Mar 6, 2025

Hi @tcojean, are you ready to merge this (I see you've left it as "draft")?

I think it's ready to merge, except for a couple of questions:

  • Are blas++ and lapack++ really required as dependencies? Building them takes a long time.
  • What testing should we add to the CI? (With blas++ and lapack++ taking so long to build, we probably don't want them to run on every push to a PR, but maybe on every push to main?)
  • Is the functional testing here sufficient, in your opinion? (e.g., we're actually testing whether the miniapp gets the right answer?)

@BenBrock
Copy link
Collaborator

BenBrock commented Mar 6, 2025

I suppose I have one more question, which is independent from functional testing, do we want to benchmark the MiniApps? (Benchmarking is a broader question, I suppose.)

@BenBrock
Copy link
Collaborator

BenBrock commented Mar 6, 2025

Including blas++ and lapack++ also turns on a bunch of warnings that makes the build very noisy. Is there any way to turn those off?

@tcojean
Copy link
Author

tcojean commented Mar 7, 2025

Hi Ben, thanks a lot for the cleanup. I think we could consider merging this.

Like you say, the BLAS++ and LAPACK++ builds take a while, so I was not sure about activating CI just yet, at least until we have our own ressources. If the 2k Github CI limit is not a hard limit, we could consider activating the CI already.

On whether they are required:

  • The CQRRPT miniapp requires LAPACK's POTRF, LACPY, QEQP3, a bunch of BLAS functions, etc. The tests also rely on a lot of LAPACK functionality. Since LAPACK++ relies on BLAS++, depending on std::linalg or something else would not change the situation here. Except if we wrote our own kernels for everything.
  • The solvers only require axpy, dot and (vector) norm, so we could at least disable LAPACK++ if we only built the solvers, and we could possibly rely on std::linalg instead.
  • If we don't change anything, I think CI only on merging to main would be the most appropriate indeed.

Note that BLAS++ and LAPACK++ also works on GPUs, so that could help pave the way for GPU miniapps, although the algorithms would need to be rewritten to rely on e.g. thrust for data management.

Tests and benchmarking:

  • I think the current tests are fine, at least they check that the solvers converge
  • I would definitely be behind adding benchmarking. I'm not sure if the solvers are the best place to add benchmarking, maybe it should be for each functionality instead (spgemm, trsv, ...)?

@tcojean tcojean marked this pull request as ready for review March 7, 2025 08:52
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.

3 participants