v3.5.0 with R update to 4.4.3, updating deprecated Ruby functions#68
v3.5.0 with R update to 4.4.3, updating deprecated Ruby functions#68beusj wants to merge 36 commits intodegauss-org:masterfrom
Conversation
Beusj patch map
Co-authored-by: beusj <8457943+beusj@users.noreply.github.com>
Co-authored-by: beusj <8457943+beusj@users.noreply.github.com>
Configure build-deploy-release workflow to trigger on v3.5.0 tag push
Enable workflow trigger on tag push
Co-authored-by: beusj <8457943+beusj@users.noreply.github.com>
…-command Update GitHub Actions to remove deprecated save-state warnings
Updated GitHub Actions workflow to use a specific username for deployment.
|
I was debugging because this was failing. Ultimately figured out that the issue was a stale geocoding_cache without correct permissions on the host, but thought these updates were still worthwhile. |
|
thanks for the contribution, @beusj ! Build time is not a bottleneck on our end, so I don't think we should refactor the Dockerfile. This project aims for reproducibility over efficiency, so I hesitate to incorporate things like caching or dockerx. I can see the usefulness of logging in general, but don't think that it would benefit any DeGAUSS users -- indeed, most of these users are not familiar with the command line or "debugging". We aim to make DeGAUSS safe enough that logging would not be required. I also hesitate to change the user interface so much. Other tools have been created which rely on the specific output of this container, so it would likely break them. Next time, please remove/digest/summarize the AI-generated content to make it easier for us to read and think about, thanks! |
|
Hi, @cole-brokamp , I am happy to clean those items up (remove R exec logging, pull request summary). The github actions did use deprecated features that caused warnings during build, so I would suggest the versions be bumped as they will likely fail in the future. I will leave those as is and let your team revert those changes if deemed appropriate. |
beusj
left a comment
There was a problem hiding this comment.
Updated changes to reflect comments from @cole-brokamp
|
Thank you for this contribution, even if the pull request isn't accepted, it helped me to upgrade my code in our repo. |
|
@reukiodo - happy to support Dockerfile build optimization, but think we should probably move to a new PR changes you made now available at https://github.com/beusj/geocoder/tree/refactor-docker |
|
I am perplexed why these changes build locally but doesn't seem to build with the github actions. I am also not sure about the merge upstream process, as it seems (with my limited interactions so far) they are less concerned with updated secure libraries and prefer a static point-in-time works-now code release. |
|
Hey - @reukiodo - based on the errors I see on build, I think the R packages that are being built from source have linux dependencies that aren't available at the stage where the packages are being built. (shouldn't be any secrets in these logs) |
There was a problem hiding this comment.
Pull request overview
This PR updates the geocoder DeGAUSS container to R version 4.4.3 with updated R packages and fixes deprecated Ruby functions to improve compatibility and maintainability.
Key changes:
- Updated R from 4.4.1 to 4.4.3 and renv from 0.15.4 to 1.1.5
- Updated all R package versions in renv.lock with full metadata
- Fixed deprecated Ruby functions:
File.exists?→File.exist?,Fixnum→Integer, and improved hash construction - Updated GitHub Actions to use newer versions (checkout@v4, login-action@v3)
- Added S3 location as build argument for flexible database source configuration
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Updated base image to R 4.4.3, version to 3.5.0, added S3 location build arg |
| renv.lock | Updated R version and all package versions with complete metadata |
| renv/activate.R | Updated renv activation script for version 1.1.5 |
| renv/settings.json | Added renv settings configuration file |
| renv/settings.dcf | Added sysreqs.enabled setting |
| lib/geocoder/us/database.rb | Fixed deprecated Ruby: File.exist?, Integer, modern hash construction |
| lib/geocoder/us.rb | Updated example database path |
| lib/geocoder/README.md | Added new README with repository link |
| entrypoint.R | Removed trailing blank lines |
| .github/workflows/build-deploy-release.yaml | Updated action versions, added S3 location handling |
| .github/workflows/build-deploy-pr.yaml | Updated action versions |
| .github/workflows/build-manual.yml | Added new manual build workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Build image | ||
| run: | | ||
| docker build -t "${{ env.versioned }}" -t "${{ env.latest }}" . |
There was a problem hiding this comment.
The manual build workflow does not specify GEOCODER_S3_LOCATION when building the Docker image, but the Dockerfile now requires it via the ADD instruction. This will cause manual builds to fail. The build command needs to include the build argument with an appropriate value.
This reverts commit 79ac306.
Goal of updating base R, R packages, and making slight changes to Ruby to deprecated functions.