-
Notifications
You must be signed in to change notification settings - Fork 180
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
install texlive conditionally #750
Conversation
See #743 (comment) I apologize for the poor quality of the review. https://github.com/rocker-org/rocker-versioned2/wiki/r-ver_4c21e4362fbf |
See #751 |
|
||
if [[ -x "/usr/bin/latex" ]]; then | ||
echo "texlive already installed" | ||
exit 0 |
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.
Why skip all other processes? In that case, the package installation will not take place, right?
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 believe any of the processes listed inside install_texlive.sh
ought to take place if we're using the ubuntu binaries which already supply these.
It's really unfortunate that the regression test was working correctly and we ended up merging. |
I don't think the current structure is really good. Ideally, we should only rely on all images |
I think the failing test will succeed once https://github.com/rocker-org/rocker-versioned2/actions/runs/7634476675/job/20798413557 is finished. I'll leave this as a draft until then. The current test structure seems to depend on testing against the current published version of the To test, I'm using a dummy Dockerfile that basically uses your proposal in #754 and adds the extra script tests:
Anyway, let's see if the above tests pass once the hotfixes you so nicely did reach DockerHub. |
@cboettig No, the Dockerfile used in the tests uses the latest scripts. rocker-versioned2/tests/rocker_scripts/Dockerfile Lines 5 to 10 in 1828862
The test case for not removing build deps is not covered here, so if CI goes green here, the |
You should add the test case |
@eitsupi thanks for all your help here, sorry I don't always understand things. Yes, I see the test case copies the scripts, but it looks to me that is based on pulling r-ver from Dockerhub, and is not running install_R_source.sh script again. Thus it appears to me the test case is grabbing the version of r-ver on Dockerhub that does not have builddeps purged. We see here in the failing test, it pull from hub, copy the new scripts, and run only the I agree we need additional tests as well. I think we actually have to test both |
Correct. As you know, install_R_source.sh is quite time-consuming to execute, so regression tests are rarely triggered. The test is here, but it is not triggered in this PR. rocker-versioned2/.github/workflows/r-build-test.yml Lines 56 to 91 in 1828862
|
looks like the above test failures are network issues (HTTP 500 errors) on the manual install for texlive. Given how temperamental the network is on https://mirror.ctan.org/ and how large the cuda images are to begin with, I'm not sad to be getting texlive from the much more reliable ubuntu repos. The changes in this PR should make the relevant scripts all more robust to working either with the hand-selected packages via tlmgr or the more standard (but larger) use of the official ubuntu apt binaries. For the reason we discussed above, these tests are still not 100% correct when running on the CI, because they are not running on the r-ver and cuda images that would be built here. I think a more complete check that I'm using locally is the above Dockerfile that runs these scripts in top of a r-ver rebuilt from scratch, and the same thing but for the cuda side. I've tested both of those builds locally. We can tickle those checks again on the CI as well, but as a hotfix to correct the previous broken builds I think this will now succeed, modulo the stability of CTAN.org servers.... |
@eitsupi ok tests are green, including I think the additional test you mentioned. I think this is ready for review, thanks for all the help! |
Co-authored-by: eitsupi <[email protected]>
Co-authored-by: eitsupi <[email protected]>
Thanks, but is this a hotfix? |
I have had several workflows that are broken until this is fixed. I am glad that we are respectful of the resources that Microsoft is providing for free, but I am also spending quite a bit of time working around the broken images at the moment and this probably impacts other users as well. |
It's complicated but I think "the community as a whole" is also overdoing it on builds, and CI runs, and whatnot. (This comment is not about Rocker, or a stack, but more generic. E.g. I have a beef with "kids these days" being lead to cookie-cutter development that runs each commit over a matrix of five or more builds for really no apparent reason. My hill to to die on I guess.) Then again we are rounding error compared to, say, bitcoin mining and LLM training. |
@eddelbuettel yeah I totally hear you that. I think your point is that CI should be used thoughtfully, not mindlessly, otherwise developers and contributors time is not well spent. I don't think anyone is worried that one of the 'magnificent 7' most profitable companies needs our help to ensure it remains financially viable. There is of course a real carbon footprint to mindless CI (and needless use of chatbots etc), despite the aggressive carbon targets... but I digress, I'm just hoping to get tensorflow working smoothly and other such bits of the python+gpu experience on our image during this brief moment when I have a few cycles to invest on this before other things take over all my time again. |
As an (index fund owning hence indirect) MSFT shareholder I am not too worried about their profitability. Free resources tend to be a cost of user acquisition that gets amortised in the long run over eventual fees. It's the carbon footprint from endlessly running jobs just because we can... But yep thanks for making it easy to deploy gpus. I appreciate it too per my recent bug report. |
@eitsupi my apologies, I didn't realize my local test was pulling the upstream image. The tex issue is specific to the changes in #743 -- specifically, the official ubuntu repo texlive binaries are installed and purged in the BUILDDEPS list. By not purging builddeps, we had conflicting versions of texlive.
This PR modifies the
install_texlive.sh
to not do the manual install of tlmgr from CPAN if the texlive has already be installed by apt-get (i.e./usr/bin/latex
already exists). I believe it should resolve the issue.