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 missing dependency on CUBLAS (#10807) #1481

Conversation

bartlettroscoe
Copy link
Contributor

CC: @vqd8a

@lucbv

This should fix link errors on an ATS2 configuration missing cublas link symbols from KokkosKernels objecct files (see trilinos/Trilinos#10807).

The customer @vqd8a confirmed this fixes is build problem in trilinos/Trilinos#10808 (comment).

This is the identical commit to that in trilinos/Trilinos#10808 made directly against Trilinos 'develop'.

I would recommend that the PR trilinos/Trilinos#10808 gets merged right away and also merge this PR so that the next time kokkos-kernels gets snapshotted into Trilinos 'develop', then it will not overwrite the changes from trilinos/Trilinos#10808.

Copy link
Contributor

@lucbv lucbv left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fix

@vqd8a
Copy link
Contributor

vqd8a commented Jul 29, 2022

@lucbv Should this PR be merged to the master branch or develop branch?

@lucbv lucbv changed the base branch from master to develop July 29, 2022 23:04
@lucbv
Copy link
Contributor

lucbv commented Jul 29, 2022

@vqd8a good point, I just changed the root branch for the merge. I see that it creates a lot of conflicts, I might just create a new PR this weekend against develop with the changes for cublas.

@kokkos-devops-admin
Copy link

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@bartlettroscoe
Copy link
Contributor Author

@lucbv Should this PR be merged to the master branch or develop branch?

I did not realize that kokkos-kernels was using a develop/master workflow.

This is really strange because now this PR shows 99 commits. Is the 'develop' branch behind the 'master' branch? How can this be?

@lucbv
Copy link
Contributor

lucbv commented Aug 1, 2022

@bartlettroscoe my guess is it is due to our release process during which we create a release branch that contains fixes that eventually make it back to the develop branch.
@ndellingwood is the right person to ask though.

@bartlettroscoe
Copy link
Contributor Author

@bartlettroscoe my guess is it is due to our release process during which we create a release branch that contains fixes that eventually make it back to the develop branch. @ndellingwood is the right person to ask though.

The 'develop' branch should always contain the 'master' branch in the develop/master workflow.

@PhilMiller
Copy link
Contributor

PhilMiller commented Aug 5, 2022

@bartlettroscoe my guess is it is due to our release process during which we create a release branch that contains fixes that eventually make it back to the develop branch. @ndellingwood is the right person to ask though.

The 'develop' branch should always contain the 'master' branch in the develop/master workflow.

That's generally been my understanding, so as to make git usage go smoothly, but that's not what Kokkos or Kokkos Kernels actually do. Perhaps they can be convinced to git checkout develop; git merge -s ours master after each release to make this so.

Edit: That should be git merge -X ours master so that changes to things like the changelog/history file do get incorporated.

@bartlettroscoe
Copy link
Contributor Author

Edit: That should be git merge -X ours master so that changes to things like the changelog/history file do get incorporated.

The document gitworkflows(7) explains how to manage a release branch using the merge workflow.

The document A successful Git branching model (Git Flow) describes how to merge 'master' back into 'develop' and resolve the conflicts when merging back. This is explained here which says:

This step may well lead to a merge conflict (probably even, since we have changed the version number). If so, fix it and commit.

(Given that you have commits on 'master' it sounds like you are actually following Git Flow.)

So the rule is that everytime you make a commit on a release branch (or 'master' if that is your release), then you merge the release branch (or 'master') back to 'develop' and be careful what you bring back.

@PhilMiller
Copy link
Contributor

It looks like the last time that happened on Kokkos was with the 2.9.00 release. There have been no merges from master/release tags from 3.0 onwards

@bartlettroscoe
Copy link
Contributor Author

bartlettroscoe commented Aug 5, 2022

It looks like the last time that happened on Kokkos was with the 2.9.00 release. There have been no merges from master/release tags from 3.0 onwards

Then how do those fixes get back to 'develop'. I someone manually cherry-picking each and every commit?

Where is the Kokkos and KokkosKernels release process documented for how branches and managed?

@PhilMiller
Copy link
Contributor

There's this document that doesn't match what's actually been done in the last few release cycles that I've been around for (3.5, 3.6, in progress 3.7):
https://github.com/kokkos/kokkos/blob/master/doc/kokkos-promotion.txt

We're been branching release-candidate-3.x.00 from develop, and then working with that, while subsequent feature work moves forward against develop. Fixes for discovered bugs get made in either develop or the RC branch, and cherry-picked to the other.

Eventually, the RC branch gets merged to master, the merge commit gets tagged, and the snapshot of that gets submitted to a Trilinos PR.

@crtrott
Copy link
Member

crtrott commented Aug 5, 2022

We have NOT done a master-develop workflow in a long time. The master branch is simply a way to ensure that someone who checks out the repo gets the latest release by default.

@bartlettroscoe
Copy link
Contributor Author

We have NOT done a master-develop workflow in a long time. The master branch is simply a way to ensure that someone who checks out the repo gets the latest release by default.

@crtrott, if this is not the master-develop workflow or Git Flow, then what is this? How can 'master' contain commits that are not in 'develop'? That does not make sense, unless you are cherry-picking commits which has a lot of downsides as explained in gitworkflows(7) that says:

There are two main tools that can be used to include changes from one branch on another: git-merge(1) and git-cherry-pick(1).

Merges have many advantages, so we try to solve as many problems as possible with merges alone. Cherry-picking is still occasionally useful; see "Merging upwards" below for an example.

Most importantly, merging works at the branch level, while cherry-picking works at the commit level. This means that a merge can carry over the changes from 1, 10, or 1000 commits with equal ease, which in turn means the workflow scales much better to a large number of contributors (and contributions). Merges are also easier to understand because a merge commit is a "promise" that all changes from all its parents are now included.

There is a tradeoff of course: merges require a more careful branch management. The following subsections discuss the important points.

If you understand Git, then merge workflows are the easier, less labor intensive, and safer way to do your branch management (because you can't forget to cherry-pick a given commit). Git cherry-pick should be a last resort (i.e. in case you fixed a bug on 'develop' that needs to impact an existing release branch).

@PhilMiller
Copy link
Contributor

@bartlettroscoe have a look at the commit graph history of master - it's mostly merges of branches that came from develop. There's always just a handful of cherry-pick commit for late fixes

@bartlettroscoe
Copy link
Contributor Author

@bartlettroscoe have a look at the commit graph history of master - it's mostly merges of branches that came from develop. There's always just a handful of cherry-pick commit for late fixes

@PhilMiller, if you are trying to keep a stable 'master' without bringing in everything from 'develop', you would create a 'hotfix' branch off of 'master', make your fix, and then merge 'hotfix' into 'master' and then merge 'master' into 'develop'. If you are creating branches off of 'develop', and merging those branches into 'master', is that not just the same thing as updating 'master' from an updated version of 'develop'? Otherwise, I don't understand this workflow. This does not seem to match any of the basic building blocks for Git workflows.

@PhilMiller
Copy link
Contributor

The release-candidate-3.x branches start from develop at a point when it's considered feature-complete and relatively stable. They have a lifetime of a few weeks of stabilization work and bug fixes, generally for problems revealed in the process of testing integration into Trilinos. Those fixes may happen on develop or the RC branch, and get cherry-picked to the other. When that's all satisfactory, the RC branch gets merged into master and tagged as the release. This avoids any 'freeze' of develop while a release is being stabilized.

It's definitely not a 'standard' workflow that is broadly used, but it's clearly a workable one, since Kokkos has been using it for a few years now without undue grumbling.

@bartlettroscoe
Copy link
Contributor Author

bartlettroscoe commented Aug 6, 2022

It's definitely not a 'standard' workflow that is broadly used, but it's clearly a workable one, since Kokkos has been using it for a few years now without undue grumbling.

@PhilMiller, actually, what you describe above is "Gitflow" except you are not merging back to 'develop'.

The problem with Git Flow is that you can only support the most current release of your software. You can't support older releases. Therefore, no-one really uses Gitflow anymore and it is considered a legacy workflow (see this page from Atlassian).

The problem with using a non-standard workflow is that it will just confuse people how know Git.

@PhilMiller
Copy link
Contributor

Kokkos only really supports the most current release it's made anyway, so that in itself isn't a problem.

Given that I'm not one of the lead developers of Kokkos, and there's been some internal talk about shifting the workflow model the project follows, I'll step away and let them discuss further if they want to. I sent them the PR link.

For the sake of moving this PR forward, the standard request is to please cherry-pick your commit on top of develop and force-push your branch.

@bartlettroscoe
Copy link
Contributor Author

For the sake of moving this PR forward, the standard request is to please cherry-pick your commit on top of develop and force-push your branch.

Yea, that is what I will do.

This should fix link errors on an ATS2 configuration missing cublas link
symboles (see #10807).
@bartlettroscoe bartlettroscoe force-pushed the tril-kokkos-kernels-cublas branch from 4d66d4c to a179da7 Compare August 6, 2022 01:24
@kokkos-devops-admin
Copy link

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@bartlettroscoe
Copy link
Contributor Author

For the sake of moving this PR forward, the standard request is to please cherry-pick your commit on top of develop and force-push your branch.

FYI: Instead of that, you can do it while on the topic branch in one command:

$ git fetch github
$ git rebase --onto github/develop HEAD^

See the git rebase --onto command at:

And that is what I did and forced pushed.

@PhilMiller
Copy link
Contributor

Oops, this was mooted by #1482. It can be closed, as it's now just a whitespace change

@bartlettroscoe
Copy link
Contributor Author

Oh well

@lucbv
Copy link
Contributor

lucbv commented Aug 8, 2022

Sorry, I meant to mention PR #1482 here... the equivalent PR to Trilinos should be merge when possible otherwise this will be taken care of when we integrate 3.7.0 in Trilinos.

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.

6 participants