I was going to make a follow up Stan commit from the Math commit here https://github.com/stan-dev/math/pull/521 (adding the Stan interface for the stuff)
Should I update the math submodule in the Stan repo with my pull request? It looks like the current submodule doesn’t quite point to the head of stan-dev/math (which will be required for the new code to build). When I pull down a fresh math submodule, it by default points to a few commits back [5a15e4, April 13th, seantalts].
If so, I assume I just check out the develop branch of the submodule and add it to the commit? (develop has the changes I need)
Just wanting to be clear. I’ve been confused by submodules before haha.
I’d say go ahead and update the submodule and get your work done. This is something that’s easy to change on a standing PR and it should not be holding you back from getting actual dev’ing done. I looked at our Dev process wiki and the exact procedure is not documented there. I’ve done a couple of these kinds of PR’s but I still don’t remember the exact procedure.
Cool beans, yeah, that’s what I ended up doing. I didn’t even notice I had a bunch of submodule changes until I went to make the pull request :p (I have since cleaned them out).
One thing Stan has taught me is adding single files at a time to a commit!
Hahaha, guilty. Gotta get off that commit -a :p.
So you’re on
develop and noticing that the
lib/stan_math submodule is pointing to an older commit than current
develop in the
math repo, is that right? I’m still getting my head wrapped around the multi-repo idiosyncrasies here but I think there’s a Jenkins job that automatically creates pull requests to update the
math submodule pointer I think whenever tests pass on
develop - creating something like this: https://github.com/stan-dev/stan/pull/2282 And then I think that’s where automation ends - I think at that point it might be up to someone with merge permissions to merge that pull request based on their judgement.
To slightly hijack this thread, this seems like an overly manual process - we only merge into
develop when tests pass on upstream as well, so we probably don’t need a manual review of the pull request. I think it makes more sense to have the submodule track the head of the parallel branch in the repos downstream -
stan/develop always pulls in the latest of
math/develop, etc. Anyone have thoughts about that?
We have a little bit of trouble with GitHub’s permissions. We currently
have develop as a protected branch. There should be able to bypass the
protection if we give Jenkins the right set of permissions.
So you’re on develop and noticing that the lib/stan_math submodule is pointing to an older commit than current develop in the math repo, is that right?
Oh! It looks like https://github.com/stan-dev/stan/pull/2282 has my stuff in it. So I’ll just wait until this goes through.
It’s through. We auto-spawn a pull request from lower level libraries to higher level libs on which they depend. We want to do all submodule updates this way. It’s fast, so usually not much lag at all.
Please don’t directly commit submodules.
Yeah, I remembered you said not to do that before so I was pretty gun-shy. Thanks!
What’s wrong with messing up submodules on a branch? You can always fix it
and squash before the PR is finalized.
I really meant don’t include them in pull requests.
Just a failure of imaginination on my part — when would you want to do that on purpose?
You want to update some stuff in Stan versus a different math but you don’t
plan on finalizing a PR based on the work. I often muck around in a test
repo without worrying about our dev process and then when I know I have
stuff working I cleanly clone the repo I want to update in a separate
directory and copy over the few files that are really going to change.
Then I add/commit the files individually and follow dev process and deal
with things like cpplint. Makes it easier to follow the strict dev process
without having it slow me down. I’d never get anything done in Stan
I’ve just added a bit of code that will hopefully auto-bump the math library from now on, though I need to double check that it works the next time someone merges into