Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve Handling of
cpp_options
#1022base: master
Are you sure you want to change the base?
Improve Handling of
cpp_options
#1022Changes from all commits
8428615
51d7302
3740633
3335166
0eb126e
4847bd2
445439b
960f056
5516a74
5ac34d9
33d20e1
b33c057
3279fa6
3d6348a
4f7fecf
7c26229
904fcc4
777a696
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Just to make sure I understand, this expects this to not say, "mock-compile-was-called"?
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.
Overall what I'm trying accomplish here is basically mimic python's
assert_called
andassert_not_called
. (https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called).Logic is:
expect_mock_compile
will pass if mock_compile is called (at all, doesn't matter how many times) and fail if mock_compile is never called.expect_no_mock_compile
is the inverse. It passes if mock_compile is not called at all and fails if mock_compile is called (even once).Implementation is:
mock_compile
emits a message with the contentsmock-compile-was-called
.expect_mock_compile
checks for this message: passes if it detects such a message, fails if it does not.expect_no_mock_compile
fails if a message with exactly this text is detected and passes otherwise. A message with any other text does not impactexpect_no_mock_compile
.There is probably a more elegant way to achieve this. Open to suggestions.
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.
There may be, but I can't think of one at the moment. I would go ahead with this implementation, unless @SteveBronder has a better idea.
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.
That's fine thanks for the clarification. Can you add a little comment (the above) just so people in the future can know what to expect?
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.
Could we keep the line numbers to less than 85 or so? Runs over my screen here. @jgabry does cmdstanr have a linter or styler setup?
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.
We haven't been running any linters on the code, although we could start doing that. Either way, I agree about keeping the lines short. There are likely some other places in the code where that needs to be fixed, although I've tried to keep them under 80 in the code I've written.
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.
yeah, since it doesn't seem like a linter was run in the past, I didn't run one on my code either, but that makes it surely inconsistent. I think there is a way to run linter just on the diff so at least we don't make the problem worse. I can look into it.
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.
If that's simple to do then that sounds good, otherwise don't worry about it. We can (and probably should) start running a linter on the whole package, maybe after merging this PR.
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.
TBH - I wouldn't advise that. It basically creates a huge diff so your subsequent blames become less meaningful. I think there is a way to exclude certain commits from blame...but....
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.
Hmm yeah that’s a good point
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.
We did this years ago in the stan math library. We essentially have a huge file that tells the compiler to ignore any auto changes.
For cmdstanr we could set up a hook so that the linter runs before any future commits. So we would put the one global change in the
.git-blame-ignore-revs
then have a git hook that runs precommit to run the linterhttps://github.com/stan-dev/math/blob/develop/.git-blame-ignore-revs
But again, seperate issue from the PR
@katrinabrock if you can try to be mindful of not having too long of lines that would be great, but no need to go through counting column
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.
Sorry I'm just not familiar with
testthat
's mocking schema. Ismod
here themod
from line 36?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.
um....good question. I don't think so because that shouldn't be in scope anymore. Which begs the question "where is this
mod
object coming from. I'll look into it.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.
lol thank you. Odd!!
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.
What is the side effect at issue here? I am just reading the code right now but tomorrow should have time to run the tests
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.
😬 That's a mystery I didn't solve completely. Some test above was causing a side effect. (I think I have a version of the code somewhere that marked which test that was.) I realized actually there could be many such cases that are not caused by my change, but revealed by my change. In this situation (and some others), I "fixed" the problem by added
force_recompile = TRUE
to the impacted test instead of actually cleaning up the culprit test.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, based on looking at my git history for when I was attempting to debug this. This is the test causing this particular side effect:
https://github.com/stan-dev/cmdstanr/blob/master/tests/testthat/test-model-compile.R#L85
So if you uncomment force_recompile, there will be a failure. If you then comment out the above linked test, the later test will pass without recompilation. (If I recall correctly.)
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 my. Okay it sounds like something is carrying state through the tests. I had another project yesterday to work on. Friday I can give this a whirl
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 thing that's carrying state through tests is the binary....because recompiling fixes it.