Fix circular deps by tcojean · Pull Request #278 · ginkgo-project/ginkgo

tcojean · 2019-03-21T12:02:39Z

This PR fixes detected circular dependencies and improves the CI.

The approach used here is to keep the current system and enforce that no kernel module ever use any function with implementation in the core module. That was the case for two things in particular, the static variables of the CudaExecutor and the GMRES solve_x function. The jacobi CUDA advanced_apply is borderline, I am somehow not able to make it throw an error. In fact, the CI is able to find some problem with the Jacobi (CUDA 9.2, GCC 7, Release build only), so it also requires fixing.

I really need the confirmation of @hartwiganzt here (or any mac user): does this build with -DBUILD_SHARED_LIBS=ON?

Changes

Add a CI step to detect circular dependencies between Ginkgo modules.
GMRES used dense matrices and uses the preconditioner apply. These calls are
moved to the core module
The static variables for the CudaExecutor were placed in the core module. They
are moved to the Cuda module.
Fix the jacobi circular dependency using some code duplication.
- All the apply kernels and the related kernel selector are duplicated with an
  alpha parameter in the calls
- The advanced version scale by alpha inside the kernel themselves.
- Use a new multiply_vec_add instead of multiply_vec which preserves the
  value in x by storing with addition.
Reduce the amount of CI jobs and integrate static builds.
- For this, we vary more than one parameter at once. Here is the new overview:
- core + gcc + debug + static
- core + clang + release + shared
- omp + gcc + release + shared
- omp + clang + debug + static
- all + gcc + cuda 9.0 + release + shared
- all + clang + cuda 9.0 + debug + static
- all + gcc + cuda 9.1 + release + static
- all + clang + cuda 9.1 + debug + shared
- all + gcc + cuda 9.2 + debug + shared
- all + clang + cuda 9.2 + release + static
- all + gcc + cuda 10.0 + debug + static
- all + clang + cuda 10.0 + release + shared

Closes #203.

Closes #270.

TODO

Add a CI step which should fail when Ginkgo has circular dependencies
Fix the existing circular dependencies
Add a static library build to the CI system and try to trim the amount of configurations by varying more than one parameter per configuration
Add a note to the contributing guideline on this circular dependency problem
Fix Jacobi circular dependency

tcojean · 2019-03-21T12:58:33Z

This how it looks like when there are circular dependencies in Ginkgo:
https://gitlab.com/ginkgo-project/ginkgo-public-ci/pipelines/52941979

The pipeline step no-circular-deps fails with the detected problems, which are then fixed in the next commit.

thoasm

Just something that I saw when scanning over the changes.
It is very neat that these small changes might resolve the circular dependencies.

cuda/solver/gmres_kernels.cu

thoasm · 2019-03-21T13:28:43Z

@tcojean I am not sure if it related to this, but I remember resolving similar problems when I fixed a linker error with test cases in #199, where I brute forced the linker to accept linking against all ginkgo libraries again. It also might have been an issue with circular dependencies, so maybe we can remove these additional link targets with this PR.

hartwiganzt · 2019-03-21T15:10:50Z

Works fine on MacOS

tcojean · 2019-03-21T15:12:11Z

@hartwiganzt just to confirm, with shared libraries, i.e. -DBUILD_SHARED_LIBS=ON (or at least no OFF parameter there) ?

hartwiganzt · 2019-03-21T15:27:14Z

Confirm.

tcojean · 2019-03-21T16:51:40Z

I also added to the PR a reduction of the amount of CI jobs (six less) and added CI jobs for static builds. I do this by shuffling around (manually) the parameters between the different builds so that we try to test as much as possible overall without having one job for each single parameter change. This is also now more balanced compared to before where we had a lot of debug tests compared to the rest.

yhmtsai · 2019-03-22T12:14:11Z

it also works fine on my MacOS, with default settings but no benchmark, no examples.
But it seems to be failed on the CI.

tcojean · 2019-03-22T12:23:06Z

@yhmtsai thanks for the input Mike.

About the benchmarks, this is related to #270 and #274.

Indeed, the CI fails because there is still one circular dependency left in jacobi. Weirdly enough, it is only triggered in some particular case (clang 9.2, gcc 7.x, release build, ...), but it is there. I will fix this and this should be able to be merged afterwards.

tcojean · 2019-03-27T17:55:34Z

The CI will hopefully confirm, but the block jacobi circular dependency should now be fixed.

Here is a summary of what I did:

Fix the jacobi circular dependency using some code duplication.

All the apply kernels and the related kernel selector are duplicated with an
additional alpha parameter in the calls.
The advanced version scale by alpha inside the kernels themselves.
Use a new multiply_vec_add instead of multiply_vec which preserves the
value in x by storing with addition.

+ GMRES used dense matrices and uses the preconditionner apply. These calls are moved to the core module + The static variables for the CudaExecutor were placed in the core module. They are moved to the Cuda module.

For this, we vary more than one parameter at once. Here is the new overview: + core + gcc + debug + static + core + clang + release + shared + omp + gcc + release + shared + omp + clang + debug + static + all + gcc + cuda 9.0 + debug + shared + all + clang + cuda 9.0 + release + static + all + gcc + cuda 9.1 + debug + static + all + clang + cuda 9.1 + release + shared + all + gcc + cuda 9.2 + release + shared + all + clang + cuda 9.2 + debug + static + all + gcc + cuda 10.0 + release + static + all + clang + cuda 10.0 + debug + shared

…ate_qy`.

See #199.

+ All the `apply` kernels and the related kernel selector is duplicated with an alpha call + The advanced version scale by alpha inside the kernel + Use a new `multiply_vec_add` instead of `multiply_vec` which preserves the value in `x` by storing with addition.

tcojean · 2019-03-28T10:36:55Z

I re-requested reviews because there is the whole jacobi fix part which is entirely new.

thoasm

Thank you for fixing this!
I have some comments, including a possible solution to the code duplication, but that is totally optional and up to you.
But I would strongly suggest moving the creation of {before,after}_preconditioner.

core/solver/gmres.cpp

cuda/preconditioner/jacobi_kernels.cu

cuda/components/warp_blas.cuh

cuda/preconditioner/jacobi_kernels.cu

thoasm

Just the documentation for the change to multiply_vec is missing, everything else looks very good!

cuda/components/warp_blas.cuh

+ For GMRES, only initialize the temporary `before_preconditioner` and `after_preconditioner` once. + For Jacobi, use only one `multiply_vec` function where the operation is parametrized by a lambda.

To test what happens.

Stuff has changed.

thoasm

LGTM!

pratikvn

LGTM!

tcojean added this to the Ginkgo 1.0.0 release milestone Mar 21, 2019

tcojean self-assigned this Mar 21, 2019

tcojean requested review from hartwiganzt, pratikvn, thoasm and yhmtsai March 21, 2019 12:02

tcojean changed the title ~~Fix circular deps~~ WIP: Fix circular deps Mar 21, 2019

thoasm reviewed Mar 21, 2019

View reviewed changes

cuda/solver/gmres_kernels.cu Show resolved Hide resolved

cuda/solver/gmres_kernels.cu Outdated Show resolved Hide resolved

cuda/solver/gmres_kernels.cu Outdated Show resolved Hide resolved

hartwiganzt previously approved these changes Mar 21, 2019

View reviewed changes

tcojean force-pushed the fix_circular_deps branch 3 times, most recently from aa17b61 to cfffeac Compare March 21, 2019 16:44

tcojean force-pushed the fix_circular_deps branch from bfceb57 to 1a45c36 Compare March 21, 2019 17:23

tcojean added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Mar 21, 2019

tcojean changed the title ~~WIP: Fix circular deps~~ Fix circular deps Mar 21, 2019

yhmtsai mentioned this pull request Mar 27, 2019

Update the third party packages to the latest releases #274

Merged

tcojean added the 1:ST:do-not-merge Please do not merge PR this yet. label Mar 27, 2019

tcojean removed the 1:ST:do-not-merge Please do not merge PR this yet. label Mar 27, 2019

Terry Cojean added 7 commits March 27, 2019 19:08

Add a CI step to detect circular dependencies between Ginkgo modules.

422eb32

Fix detected circular dependencies.

716f0c0

+ GMRES used dense matrices and uses the preconditionner apply. These calls are moved to the core module + The static variables for the CudaExecutor were placed in the core module. They are moved to the Cuda module.

Update nocuda container to actually use clang 7.0

7523225

Remove unused x from gmres step_2 and change solve_x to `calcul…

40d4a33

…ate_qy`.

Remove now unneeded extra links for the tests.

7b595aa

See #199.

tcojean force-pushed the fix_circular_deps branch from 34df22b to 1ceb97f Compare March 27, 2019 18:09

tcojean requested review from hartwiganzt and thoasm March 28, 2019 10:36

thoasm requested changes Mar 28, 2019

View reviewed changes

core/solver/gmres.cpp Outdated Show resolved Hide resolved

core/solver/gmres.cpp Outdated Show resolved Hide resolved

cuda/preconditioner/jacobi_kernels.cu Show resolved Hide resolved

cuda/components/warp_blas.cuh Outdated Show resolved Hide resolved

thoasm reviewed Mar 28, 2019

View reviewed changes

cuda/preconditioner/jacobi_kernels.cu Show resolved Hide resolved

thoasm previously approved these changes Mar 28, 2019

View reviewed changes

cuda/components/warp_blas.cuh Show resolved Hide resolved

Improve the fixes of both GMRES and Jacobi.

0ddff67

+ For GMRES, only initialize the temporary `before_preconditioner` and `after_preconditioner` once. + For Jacobi, use only one `multiply_vec` function where the operation is parametrized by a lambda.

tcojean force-pushed the fix_circular_deps branch from d4d8b69 to 0ddff67 Compare March 28, 2019 13:41

thoasm self-requested a review March 28, 2019 16:42

thoasm approved these changes Mar 28, 2019

View reviewed changes

pratikvn approved these changes Mar 28, 2019

View reviewed changes

tcojean merged commit 4071a2e into develop Mar 28, 2019

tcojean deleted the fix_circular_deps branch March 28, 2019 19:18

tcojean mentioned this pull request Mar 28, 2019

fails to build on macOS with disabled cuda #270

Closed

ginkgo-bot added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Apr 17, 2019

Conversation

tcojean commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Closes #203.

Closes #270.

TODO

Uh oh!

tcojean commented Mar 21, 2019

Uh oh!

thoasm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thoasm commented Mar 21, 2019

Uh oh!

hartwiganzt commented Mar 21, 2019

Uh oh!

tcojean commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hartwiganzt commented Mar 21, 2019

Uh oh!

tcojean commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhmtsai commented Mar 22, 2019

Uh oh!

tcojean commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcojean commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcojean commented Mar 28, 2019

Uh oh!

thoasm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thoasm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thoasm left a comment

Choose a reason for hiding this comment

Uh oh!

pratikvn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tcojean commented Mar 21, 2019 •

edited

Loading

tcojean commented Mar 21, 2019 •

edited

Loading

tcojean commented Mar 21, 2019 •

edited

Loading

tcojean commented Mar 22, 2019 •

edited

Loading

tcojean commented Mar 27, 2019 •

edited

Loading