Skip to content
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

move plot grid function and class from ospsuite #246

Merged
merged 4 commits into from
Apr 23, 2022
Merged

Conversation

IndrajeetPatil
Copy link
Member

@IndrajeetPatil
Copy link
Member Author

Not sure, why there are these issues with installing pandoc now.

Failures
 - pandoc (exited 1) - pandoc not installed. An error occurred during installation:
 The remote server returned an error: (429) Too Many Requests. Too Many Requests
Command exited with code 1
7z a failure.zip *.Rcheck\*
7-Zip 21.07 (x64) : Copyright (c) 1999-2021 Igor Pavlov : 2021-12-26

@Yuri05
Copy link
Member

Yuri05 commented Apr 23, 2022

"Too Many Requests" probably server too busy.
I restarted the build manually and it seemed to work (at least with pandoc) - so was just a one time glitch.

Now the build fails only due to the library ospsuite statement in one of the code examples. Once it is removed - should be fine.

R/plot-grid.R Outdated
#'
#' @examples
#'
#' library(ospsuite)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove #' library(ospsuite)

R/plot-grid.R Outdated
#' @description
#'
#' R6 class defining the configuration for `{patchwork}` plot grid used to
#' create a grid of plots from `{ospsuite}`. It holds values for all relevant
Copy link
Member

@Yuri05 Yuri05 Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove from {ospsuite} or replace it with from {tlf}

@IndrajeetPatil
Copy link
Member Author

@Yuri05 Removed the references to ospsuite, which were left in by mistake.

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2022

Codecov Report

Merging #246 (f97ab33) into develop (52f14e0) will decrease coverage by 0.34%.
The diff coverage is 5.00%.

@@             Coverage Diff             @@
##           develop     #246      +/-   ##
===========================================
- Coverage    52.01%   51.66%   -0.35%     
===========================================
  Files           50       51       +1     
  Lines         2686     2706      +20     
===========================================
+ Hits          1397     1398       +1     
- Misses        1289     1308      +19     
Impacted Files Coverage Δ
R/plot-grid.R 5.00% <5.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52f14e0...f97ab33. Read the comment docs.

IndrajeetPatil added a commit to Open-Systems-Pharmacology/OSPSuite-R that referenced this pull request Apr 23, 2022
@msevestre
Copy link
Member

appveyor works but not test coverage.

@IndrajeetPatil
Copy link
Member Author

@msevestre Yes, this is same as this issue: #241

GHA is no longer working. We either need to fix it (I unsuccessfully tried in #242) or completely remove it.

@msevestre
Copy link
Member

Can we do code coverage like we do for all other packages? Tha way we don't rely on it anymore. We keep having issue with GHA and that's a real PIA

@IndrajeetPatil
Copy link
Member Author

Well, we are having issues now because we removed ospsuite.utils from CRAN, but it was working fine before.

Actually, the test coverage GHA is redundant because we are already using AppVeyor for this.
In fact, for this very PR, code coverage was successfully computed (#246 (comment)). So we can just remove GHA for test-coverage.

@msevestre msevestre merged commit dd00853 into develop Apr 23, 2022
@msevestre msevestre deleted the add_plot_grid branch April 23, 2022 20:22
@Yuri05
Copy link
Member

Yuri05 commented Apr 24, 2022

Well, we are having issues now because we removed ospsuite.utils from CRAN, but it was working fine before.

Any problems to install ospsuite.utils in GHA in the same way as in appveyor.yml?

- Rscript -e "install.packages('https://ci.appveyor.com/api/projects/open-systems-pharmacology-ci/ospsuite-rutils/artifacts/ospsuite.utils.zip', repos = NULL, type = 'binary')"

Actually, the test coverage GHA is redundant because we are already using AppVeyor for this.
In fact, for this very PR, code coverage was successfully computed (#246 (comment)). So we can just remove GHA for test-coverage.

I would prefer the other way around: remove code coverage from appveyor.yml but keep the GitHub action.
Because AppVeyor and GHA are executed in parallel. And in some repositories (e.g. Reporting Engine) Code coverage takes ages - so executing it in parallel would reduce the build time.
Or are there any advantages of code coverage in appveyor (@msevestre ) ?

@msevestre
Copy link
Member

Code coverage doesn't have to be run each time. I think we need to split it into it's own task (or have it work with GA). In general, code coverage gives you some nice metrics but isn't required to decide whether to merge a PR or not. So as for our main apps(pksim, mobi and core), I would suggest to create a specific task for this that would run on a weekly basis, especially if it takes a lot of time. Even with GHA, this is time that we do need to wait for everytime to get valuable CI feedback

@Yuri05
Copy link
Member

Yuri05 commented Apr 24, 2022

agree. running codecovr nightly/weekly/whatever is surely better than on every commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants