-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Feature/0712 model base class #713
Conversation
…ature/0712-model-base-class
…an-dev/cmdstan into feature/0712-model-base-class
…an-dev/cmdstan into feature/0712-model-base-class
These tests are going to fail until stan-dev/stan PR 2785](stan-dev/stan#2785) is merged, because the submodule won't be pointing to the right place until then. |
Sorry, this branch isn't even working on my machine. I'll ping this again when it's working. |
…an-dev/cmdstan into feature/0712-model-base-class
…ature/0712-model-base-class
…an-dev/cmdstan into feature/0712-model-base-class
This is ready to review now. |
As pointed out on discourse: "new with this PR (to me) is that now CmdStan picks up stuff which are defined in the Stan makefiles - at least this is how we should handle it. Right now the additional make targets are defined in both repos - which is not good, I think." |
pre-existing redundancies between the CmdStan and Stan makefiles should be addressed via a different issue. in this PR, the files |
Sure, pre-existing redundancies are not part of this PR - it's just that I have the impression that you do introduce additional redundancies with this PR. If that's right then you create technical debt and that should not be the case. Let me have a look into it again and maybe I can suggest something here. Anyway, right now I am getting this error:
I have cmdstan at this branch and the stan repository is moved to the branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update the compile-info
target in the makefile
. This is there to show the commands which are executed for a stan model. This now two commands.
Other than that this is really cool. Compilation times go down from 22s to 5s on my system for the bernoulli example.
make/program
Outdated
@@ -2,6 +2,7 @@ | |||
# Models (to be passed through stanc) | |||
## | |||
CMDSTAN_MAIN ?= src/cmdstan/main.cpp | |||
CMDSTAN_MAIN_O ?= src/cmdstan/main.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the patsubst command to derive from CMDSTAN_MAIN the value for CMDSTAN_MAIN_O.
make/program
Outdated
%.hpp : %.stan bin/stanc$(EXE) | ||
@echo '' | ||
@echo '--- Translating Stan model to C++ code ---' | ||
$(WINE) bin/stanc$(EXE) $(STANCFLAGS) --o=$@ $< | ||
|
||
%.d: %.hpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition here overrides the one included in the top makefile which pulls in math/make/dependencies. Could you please document that you do this here on purpose. The alternative is to not include the dependencies from math which we may not need actually for cmdstan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing the override.
we need the dependencies rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok...once you do that, then the model_header.o dependency can be build. The only downside is that then you will build the dependency thing for the stan model hpp file... which wastes 1s of compile time as it anyway gets recreated with any model update...but ok; still a win.
make/program
Outdated
$(LINK.cpp) $(CXXFLAGS_PROGRAM) -include $< $(CMDSTAN_MAIN) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(OUTPUT_OPTION) | ||
@echo '--- Compiling, linking C++ code ---' | ||
cp $*.hpp $*.cpp; \ | ||
$(LINK.cpp) $(CXXFLAGS_PROGRAM) -c -o $*.o $*.cpp ; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. this is less ideal, I think. You can make the compiler interpret the hpp file just like a c++ file with the -x c++
option. So I would suggest to replace 38-41 with this:
$(COMPILE.cpp) $(CXXFLAGS_PROGRAM) -x c++ -o $*.o $<
$(LINK.cpp) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(CMDSTAN_MAIN_O) $*.o $(OUTPUT_OPTION)
This way you pickup the right things for compilation (first line) and for linking (second line). Moreover, you do not need to do this copying and removing business.
makefile
Outdated
@@ -151,7 +151,7 @@ build-mpi: $(MPI_TARGETS) | |||
@echo '--- boost mpi bindings built ---' | |||
|
|||
.PHONY: build | |||
build: bin/stanc$(EXE) bin/stansummary$(EXE) bin/print$(EXE) bin/diagnose$(EXE) $(LIBSUNDIALS) $(MPI_TARGETS) | |||
build: bin/stanc$(EXE) bin/stansummary$(EXE) bin/print$(EXE) bin/diagnose$(EXE) $(LIBSUNDIALS) $(MPI_TARGETS) $(CMDSTAN_MAIN_O) $(STAN)src/stan/model/model_header.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model_header.d
should just be removed here, I think. At least this caused errors for me and I don't quite see the need for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can stay now once you remove the %.d override, but please verify.
makefile
Outdated
@@ -182,6 +182,7 @@ clean-manual: | |||
|
|||
clean-all: clean clean-deps clean-libraries clean-manual | |||
$(RM) -r bin | |||
$(RM) -r src/cmdstan/main.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to delete $(CMDSTAN_MAIN_O)
here and not directly name the file defined in the variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be adjusted?
…ature/0712-model-base-class
@syclik or @wds15 - most definitely failing unit tests on linux with mpi - here's the end of the error log:
what needs to be included and/or linked where? |
this appears to be the command that's failing - complaints are about
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98) |
@mitzimorris, I'm sorry -- I don't know. I looked at it, but I wouldn't know where to go for the MPI build stuff; I can read it, but I can't really maintain it. You'll have to get help from @wds15 or someone else that understands exactly how this is being built. It might be worth checking that GPU also builds independently of this failure. What should we do about the state of CmdStan and Stan? Should we revert? cc: @bob-carpenter, @seantalts |
I looked at this yesterday for a moment, but i wasnˋt able to figure it out of what is going on. I can certainly spend some time on this over the weekend, but today or tomorrow is not possible for me to do that. Generally you need to dynamically link against the boost stuff and you need to statically link in the mpi cluster inst thing...not sure what is going wrong right now. |
Re: reverting Stan - from what I understand, those changes are safe to make without this branch being merged in. The tests seem to be passing in develop too to second that idea. |
.circleci/config.yml
Outdated
version: 2 | ||
build_and_test: | ||
jobs: | ||
- manual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the docs made for CmdStan now, by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated circleci config to remove dependencies on Stan docs. now circleci checks task make manual
- tests passing.
make/program
Outdated
|
||
|
||
## | ||
# Dependencies file | ||
# Dependencies file for precompiled header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is right - these are dependency files for everything. Line 53 includes the one from the model_header file.
make/program
Outdated
$(LINK.cpp) $(CXXFLAGS_PROGRAM) -include $< $(CMDSTAN_MAIN) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(OUTPUT_OPTION) | ||
@echo '--- Compiling, linking C++ code ---' | ||
$(COMPILE.cpp) $(CXXFLAGS_PROGRAM) -x c++ -o $*.o $< | ||
$(LINK.cpp) $(LDLIBS) $(LIBSUNDIALS) $(MPI_TARGETS) $(CMDSTAN_MAIN_O) $*.o $(OUTPUT_OPTION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds dumb, but have you tried swapping the order of the link libraries? Try putting CMDSTAN_MAIN_O back one or even at the beginning of the list. I know link order does matter in weird ways sometimes.
…ature/0712-model-base-class
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.96) |
+1 |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01) |
@wds15 - got the makefiles working (thanks @seantalts! you were right about link order) - |
I think all is good... with the latest changes the Jenkins tests are not anymore run. Is that on purpose? I think they should still run. (This is major hack to ensure things are in place) |
maybe changes just to .circleci/config.yml don't kick off jenkins test? I make a trivial change to the makefile and now circleci, jenkins, and travis are all checking again. |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99) |
🎉 So glad this made it in! Down to the wire, haha. |
Submisison Checklist
./runCmdStanTests.py src/test
Summary:
Update the basic command (
command.hpp
) to instantiate the services with a base model class (stan::model::base_model
) and build a reference using a new function defined in the generated model code as of stan-dev/stan PR 2785.Intended Effect:
Allow the command to be compiled independently of the model to speed up compilation times.
How to Verify:
All functionality should still work as is.
Side Effects:
User-facing behavior is unchanged.
There may be performance issues on some platforms as we've seen one model in our test suite take longer on our old Mac pro but not on other platforms.
Documentation:
There's no user-facing documentation and both code and makefile changes are minimal.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: