prepare actions to include qc report #10

zsigmas · 2024-05-31T16:19:14Z

Compacts GH actions building process in a single file equal, with minor changes, to the one we had before
Includes QC report steps in GH actions
We lose parallel execution of jobs, (execution time is low anyways and most time of parallel jobs is consumed in pod start up anyway)
Parallel loss is outweighed, in my opinion, by a simplified GH actions flow
Because the same pod is used, all artifacts generated are available across steps this simplifes including the qc report

The build will fail as actions points to @main and those actions do not exist there yet. They will build once the PR merged

sorinvoicu · 2024-06-03T07:49:00Z

.github/workflows/check.yml

why merge check/lintr/pkgdown ?

Main point is that pkgdown uses the output of tests to generate the quality control documentation, it is simpler that moving artifacts between Github action jobs. Secondarily, personal perspective, it is simpler to maintain a single R script that does all the job than maintaining separated yaml files.

I would still separate the concerns. I can chip in on the artifacts part if you want.

Ideally we should just plug-and-play other qc reports/validatoR as the industry evolves. This ensures that.

sorinvoicu · 2024-06-03T07:49:53Z

.github/workflows/package_building.yml

      - name: Check URLs 🌐
        run: |
          lychee . --format markdown --verbose --no-progress >> $GITHUB_STEP_SUMMARY
        shell: bash  

-      - name: Checkout gh-pages branch ⬇️
-        if: github.ref_name == 'main'


why generate pkgdown docs for every commit?

Removed for debugging and creating the documentation, I will include it again

workflows.md

sorinvoicu · 2024-06-03T07:51:21Z

.github/workflows/package_building.yml


    steps:
      - name: Checkout project ⬇️
        uses: actions/checkout@v4

-      - name: Install package dependencies 📄
-        uses: boehringer-ingelheim/dv.templates/.github/actions/dependencies@main           


why is this removed?

They are now installed inside single build, but if we prefer a separate step for installing dependencies, no opposition.

sorinvoicu · 2024-06-03T07:53:48Z

pkg_pipeline/single_build.R

+message("###### INSTALLING (S) ######")
+message("############################")
+
+pak::local_install_deps(".", upgrade=FALSE, ask=FALSE, dependencies = TRUE)


I would say keep the deps step removed above (since it's a dependency in all steps, not just check/lintr/pkgdown) and comment these 2 lines

also why not add the qc report code in this repo in the first place? it uses nobody outside BI to have it in the package repo

The qc report is used inside the test files to link specs and test cases, therefore it is required when running tests. In a next iteration we can think about ways to move it to dv.templates. No problem with that.

why not do it in the first place?

sorinvoicu · 2024-06-03T07:59:51Z

pkg_pipeline/single_build.R

+message("###### CHECKING (S) ######")
+message("##########################")
+
+check_results <- rcmdcheck::rcmdcheck(error_on = "never", args = "--no-tests")


would definitely not serialize steps in a big file. KISS, remember?

why wait for fails if you can see them all at once and hopefully fix them in one go?

I would argue that it is simpler to maintain a single R file that runs all the steps and offers a final summary with all the errors found. You don't wait more than having separate pods or have less information from my point of view, a table is included in the Action Summary, and in the end you have to check the logs anyway to find the specific errors.

old wheel to a new cart. disagree. I thought the action status would save you from doing just that...

@sorinvoicu
Strongly agree with @zsigmas on this one and I'm ready to die on this hill.

"One cart carrying all the things we need VS several carts that need coordination."

be my guest @ml-ebs-ext (a name update would work as well)

I have slept on the matter and have a few comments.

My combative wording ("ready to die on this hill") stems from your unfortunate "KISS, remember?" except I'm not insulting (accidentally or purposefully) anyone. In retrospect, I could have been more civil.

On the topic of simplicity, yes, I do think one big file that executes top-to-bottom is simpler than several files strung together through github actions. It is simpler because (in subjective decreasing order of importance):

You only need to look in a single place to understand how the build process works.

Related to the previous point, all build information is available at the same time on a single pod, making its manipulation trivial.

You can run it locally without having to mirror github machinery on your machine (a luxury that our internal colleagues don't have). This means you can tweak it locally and have a pretty good sense that it's going to work without having to go through minutes-long trial and error, which not only makes us miserable but costs the company money.

We have less work to do whenever github decides to deprecate features we rely on (https://github.blog/changelog/label/actions+deprecation/)

It makes it easier for potential users to take our code and use it mostly as-is outside of github. Our choice of hosting platform should not dictate theirs.

It is more environmentally friendly, because we save the cost of spinning up several containers (drop in the ocean, I know, considering we're encouraging the use of R).

I have numbered them so that you can comment on any of them individually and see where the nature of our disagreement lies. I would also like to know your reasons for thinking that a split .R file is simpler.

On the topic of my username: I revised it from mlebs to the obviously superior ml-ebs-ext, that make it clear that I'm an external working through EBS and my initials are "m" and "l".

@sorinvoicu Sorry, I failed to mention your nickname yesterday. This is an important issue because it sits upstream of developers and dictates the way they work. I would like to hear your reasons for thinking that a split .R file is simpler. Even if we fail to convince each other, the whole team can benefit from this conversation.

prepare actions to include qc report

17bde84

zsigmas requested review from sorinvoicu and mattkorb May 31, 2024 16:19

zsigmas mentioned this pull request May 31, 2024

includes validation qc and aligns with new gh actions structure Boehringer-Ingelheim/dv.manager#8

Merged

zsigmas self-assigned this May 31, 2024

sorinvoicu requested changes Jun 3, 2024

View reviewed changes

sorinvoicu self-requested a review June 3, 2024 08:03

zsigmas added 4 commits June 3, 2024 10:33

undo documentation replacement

8655fb0

install dependencies on a separate step

794be7c

deploy documentation only on main branch

ec591b9

deploy documentation only on main branch (bis)

5f875fb

zsigmas closed this Jun 3, 2024

zsigmas deleted the refactor_actions_for_qc branch June 3, 2024 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prepare actions to include qc report #10

prepare actions to include qc report #10

zsigmas commented May 31, 2024 •

edited

Loading

sorinvoicu Jun 3, 2024

zsigmas Jun 3, 2024

sorinvoicu Jun 3, 2024

sorinvoicu Jun 3, 2024

sorinvoicu Jun 3, 2024

zsigmas Jun 3, 2024 •

edited

Loading

sorinvoicu Jun 3, 2024

zsigmas Jun 3, 2024

sorinvoicu Jun 3, 2024

sorinvoicu Jun 3, 2024

zsigmas Jun 3, 2024

sorinvoicu Jun 3, 2024

sorinvoicu Jun 3, 2024

sorinvoicu Jun 3, 2024 •

edited

Loading

zsigmas Jun 3, 2024

sorinvoicu Jun 3, 2024 •

edited

Loading

ml-ebs-ext Jun 3, 2024

sorinvoicu Jun 3, 2024 •

edited

Loading

ml-ebs-ext Jun 4, 2024

ml-ebs-ext Jun 5, 2024

prepare actions to include qc report #10

prepare actions to include qc report #10

Conversation

zsigmas commented May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsigmas Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorinvoicu Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorinvoicu Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorinvoicu Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsigmas commented May 31, 2024 •

edited

Loading

zsigmas Jun 3, 2024 •

edited

Loading

sorinvoicu Jun 3, 2024 •

edited

Loading

sorinvoicu Jun 3, 2024 •

edited

Loading

sorinvoicu Jun 3, 2024 •

edited

Loading