-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Update LBT14 to use correct denom #276
Conversation
Unit Tests Summary340 tests 252 ✅ 1m 0s ⏱️ Results for commit bcaf90b. ♻️ This comment has been updated with latest results. |
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit 914be5f ♻️ This comment has been updated with latest results. |
@shajoezhu is tlg-catalog set up to use staged dependencies in the development profile? If not, this one may have to wait until the tern PR is merged to pass checks |
it should https://github.com/insightsengineering/tlg-catalog/blob/main/package/staged_dependencies.yaml not sure why it is not picking up changes in tern? hi @cicdguy please could you take a look? |
Looks like it did use |
hi @cicdguy , i think this is not working, I made a manual vbump on https://github.com/insightsengineering/tern/pull/1326/files, v0.9.6.9007. it seems in the PR currently, it is fetching from main. So the staged.dependencies, has not worked. good thing is that scda.test is working, so we can test these changes and ensure things are working @pawelru fyi |
You are right - staged.dependencies is skipped and all the dependencies are installed from r-universe: This is why: tlg-catalog/.github/workflows/check.yml Line 43 in b64b542
https://github.com/insightsengineering/r.pkg.template/blob/da0ba8e247827b92b59edd8c064388eb55948b88/.github/workflows/build-check-install.yaml#L378-L380 The rationale is to enable WebR. WebR checks how the packages are installed and request wasm binaries accordingly. When installed from sources (e.g. via staged.deps) there were some errors - not sure if it's still the case. Is this a blocker for you? As we are right before changing this into pak-based installation - I don't think it would be wise for us to enable staged deps right now. Can you do test after merge to tern devel? |
Thanks @pawelru for the explanation, it is not a blocker for us, as we are checked via scda.test, and we can merge tern, scda.test, then update tlg-c code after. Just need to redefine the workflow, and perhaps good idea to record these somewhere. I support the change, just want to better prepare for this change, and make sure it is a smooth transtion, rather than a hardlanding, and breaking changes everywhere. If we were going to use pak for updating rtables/tern/formatters/rlistings, how should we make change during the process in tlg-c |
The good thing about that is that these are independent so that changes elsewhere (rtables/tern/...) are independent to what's here. You don't have to coordinate it across repos. |
thanks a lot Pawel! |
# Pull Request Fixes #1325 Adds `denom` to `s_count_occurrences_by_grade()` as well as `s_count_cumulative()` and `s_count_missed_doses()`. Downstream changes: - [ ] insightsengineering/scda.test#155 - [ ] insightsengineering/tlg-catalog#276 --------- Co-authored-by: shajoezhu <[email protected]> Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
@shajoezhu this is now passing! |
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.
lgtm! Thanks a lot @edelarua
Downstream changes resulting from insightsengineering/tern#1326. No other templates are affected.