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

Future of rgrass - meeting minutes #34

Open
veroandreo opened this issue Dec 14, 2021 · 25 comments
Open

Future of rgrass - meeting minutes #34

veroandreo opened this issue Dec 14, 2021 · 25 comments

Comments

@veroandreo
Copy link
Collaborator

veroandreo commented Dec 14, 2021

Date: 12/14/2021
Participants: Roger Bivand, Floris Vanderhaeghe, Valentin Lucet, Helmut Kudrnovsky & Veronica Andreo

Package naming: keep the version or drop it?
While creating a grass version specific R package makes some sense, i.e., if you have GRASS 7 you use rgrass7, when you use GRASS 8 you use rgrass8; we all liked the idea of a plain rgrass package that might either work for GRASS 7 and 8, or from 8 onward. For this latter option, it was suggested to include a note in the upcoming rgrass7 saying that the update of the package for grass8 will be called rgrass.

Testing
This is highly needed. The new rgrass version should have automatic tests to check the code in different operative systems as it is currently in place in GRASS repo through GitHub actions (might use docker images or not). We commented that we could check both against grass main and latest release branches. Valentin commented that he has set such testing environment before and he volunteered to tackle this issue.

Vector data
We are all happy with sf being the link for vector data. No changes here, in principle. The use of rsqlite as to link directly to the attribute table is to be revised.

Raster data
Several options were discussed:

  • keep it as is for now
  • keep using sp + r.in/out.bin, but drop rgdal as it will be withdrawn
  • write GRASS raster binary files into terra or stars objects. This is yet not so clear as most of us are unfamiliar with those packages, but there was some consensus that as stars uses GDAL through sf, and the two are already suggested dependencies for rgrass, stars would be a more natural and straightforward choice. Also considering that in the future grass imagery space time datasets could be converted into stars objects.

Side note on rater data: Roger explained that the use of r.in/out.bin was decided because it is "cheaper" to export and import uncompressed data rather than compress-export-uncompress-import. However, other options might be explored in the future.

Parameters and flags
Changes in parameter names and flags in GRASS 8 should be accounted for. We need to check the grass changelog for that.

Vignettes
The main problem with vignettes is that they would require GRASS and the NC sample dataset installed to be self compiled. This is not possible for a CRAN environment. Valentin said he had some experience with vignettes running uninstalled software. Roger asked if it was possible to get a GRASS binary without gui (In fedora, grass-gui is a separate package, would that help?). We could explore also the possibility of linking to the server where GRASS builds occur, we need to ask. Other option is that vignettes are just “frozen” and state that a certain environment must be set in advance for them to work. This latter seemed like the best option for now.

Where to host the new rgrass version?
We discussed several options too: personal github account, r-spatial, grass-addons or new independent organization. We agreed that Roger would create a new repo in his personal account to which we will be invited and then, we could move it to r-spatial subject to their rules. Hosting rgrass in grass-addons repo did not seem like a good option as it is not installed with g.extension in grass, but it is an R package.

Other related topics

  • Short summary of rgrass history by Roger :)
  • Current and future problems mostly in Windows related to U-CRT and the download of proj database (~500 Mb) when installing GRASS through OSGeo4W. GRASS 8 GUI is currently not working in Fedora 35, GRASS not ready for CDN.
  • In the current rgrass7 most exceptions are related to Windows.

Draft roadmap

  1. Create repo and invite others (Roger)
  2. Set up testing (Val)
  3. Drop rgdal support, keep discussing/decide what to do with raster data and implement it
  4. Develop vignettes (Vero will make an attempt)
  5. Drop unused stuff
  6. Include new functionality or enhance existing one to e.g. discover binaries, create locations from epsg codes, etc.

@rsbivand @VLucet @florisvdh @hellik please have a look and let me know if I misunderstood anything. Thanks a lot for your willingness to participate and move forward!

@hellik
Copy link
Collaborator

hellik commented Dec 14, 2021

as I've left the meeting earlier, I can't comment on all issues mentioned in the meeting notes.

though, all looks good so far.

@veroandreo I think we should link this discussion to the GRASS user ML; maybe we're able to atttract more interested people.

@VLucet
Copy link
Collaborator

VLucet commented Dec 14, 2021

Thanks @veroandreo for taking notes! Next meeting I'll take notes so we rotate roles! (sorry I came in late as well).

  1. About hosting & naming: I see @rsbivand renamed the rgrass7 repo to rgrass. I think we should actually keep rgrass7 as it is and open a new repo (named rgrass). This seems cleaner to me (isn't there a risk to breaking urls linking the repo as well?). I think the goal of this new rgrass should be to focus on supporting both GRASS 7 and 8 onward.
  2. I like the idea of a rgrass github org that would host both rgrass7 and the new rgrass. I could move my rgrassdoc there as well. I can imagine other repos being hosted there, like a workshop on applying rgrass (very long term goal of course, I just like making workshops!).
  3. Once we make a decision on naming etc. I can open a PR and set up testing using GitHub actions. I would suggest to start by (1) setting it up with linux/macos/windows with GRASS 7 running simple tests by wrapping the existing examples and (2) adding GRASS 8 to the job matrix and see what breaks and (3) writing better tests!
  4. Unsure about raster and gdal, I don't think I understand the implications of each option well enough.
  5. I like writing vignettes and could help with that of course.

@hellik
Copy link
Collaborator

hellik commented Dec 14, 2021

3. Once we make a decision on naming etc. I can open a PR and set up testing using GitHub actions. I would suggest to start by **(1)** setting it up with linux/macos/windows with GRASS 7 running simple tests by wrapping the existing examples and **(2)** adding GRASS 8 to the job matrix and see what breaks and **(3)** writing better tests!

we already have compiling and testing winGRASS in github actions:

though this process has to be updated to reflect OSGeo4W version 2

@VLucet
Copy link
Collaborator

VLucet commented Dec 14, 2021

we already have compiling and testing winGRASS in github actions:

Cool, that means I don't have to figure out how to install it on windows (action wise)! Would be cool to have it as a standalone action, I'd just have to write uses:. Could be another goal down the line.

@hellik
Copy link
Collaborator

hellik commented Dec 14, 2021

we already have compiling

compiling winGRASS to testing rgrass may not be needed as precompiled winGRASS can be downloaded via github action. e.g.

example download

Start-Process ('.\'+$exe) -ArgumentList '-A -g -k -q -s http://download.osgeo.org/x86_64 -P cairo,fftw,freetype-devel,gdal-ecw,gdal-mrsid,liblas-devel,libxdr,msys,pdcurses,python3-numpy,python3-pywin32,python3-wx,regex-devel,wxpython,zstd-devel' -Wait

@rsbivand
Copy link
Owner

rsbivand commented Dec 15, 2021

@VLucet Yes, I thought about the architecture of one or two repos. After (maybe too little) consideration, I changed the name of the repo rather than starting a new repo. My idea (maybe wrong) is that 1) I switched the default branch from master to main, and changed the URL in DESCRIPTION to point to rgrass rather than rgrass7 in main. So a release soon would be good (the current state checks OK with 7.8.6 and 8.0dev).

Next, we make an rgrass7 branch, which will be the target maintenance source for the "old" package. Until the rgrass branch is ready for release, mainand rgrass7 are the same (changes on rgrass7 are merged into main). When rgrass is ready for submission to CRAN, rgrass is merged with main, and rgrass7 is updated to give a startup message advising users to switch to rgrass.

I thought that there is so much shared code that we risk missing maintenance updates if the codebase is split over two repos. Does this make sense?

So testing would use main in principle, but could use main and rgrass before the switch, main and rgrass7 afterwards if need be.

Then when we are ready, we can create an organisation and move the rgrass and rgrassdoc repos there.

@VLucet
Copy link
Collaborator

VLucet commented Dec 15, 2021

@rsbivand Ah yes I see! That indeed makes a lot more sense in terms of maintenance, I completely agree.

@florisvdh
Copy link
Collaborator

Thank you @veroandreo for the minutes, they look complete to me.

@rsbivand yes, I also think it's sensible to use the three branches in order to maintain the two package flavours. To avoid confusion, maybe the definition of the branch contents could be put in a file .github/CONTRIBUTING.md? (It's a file also picked up by pkgdown, linked as 'Contributing guide' in the sidebar).

I think that main - although equal to rgrass7 for the time being - will also contribute to rgrass branch, if only for new organizational additions (like yesterday's commits, perhaps a few contributing guidelines), that are common to both rgrass and rgrass7 package source. There might even be a need for a separate branch (instead of main) with the common base for rgrass7 and rgrass branches, while the specific stuff happens in the latter branches (especially rgrass obviously):

  • Is it expected that rgrass7 (main) will get specific commits that should not go in rgrass, before release of the latter? (I.e. other than the finally added startup message pointing to rgrass package)
  • If main is to be effectively the rgrass7 package by definition for now (because it's the current CRAN package), and if answer to above question is 'yes': then (only then) a common codebase for both packages would need to go in a separate branch, not in main. Also, that would make an rgrass7 branch only needed at the time of rgrass release, since before that it would only duplicate main.

@VLucet
Copy link
Collaborator

VLucet commented Dec 24, 2021

I agree with @florisvdh for the CONTRIBUTING file.
Maybe I'm missing something, but as long as the rgrass branch can be rebased onmain every now and then (commits could be cherrypicked) we should not have a need for another separate branch, A main, rgrass7 and rgrass set of branches makes sense to me.

@VLucet
Copy link
Collaborator

VLucet commented Jan 19, 2022

I just wanted to start the conversation on this again, and ask whether we have decided what set of branches we'd like to work with, and if yes, whether I can get started with setting up github actions on the main and rgrass branches.

@florisvdh
Copy link
Collaborator

As far as I'm concerned, please feel free to choose the setup that you expect will work best. My suggestions were just meant to aid the thinking process!

@rsbivand
Copy link
Owner

Could someone add the CONTRIBUTING file to main, then I'll create the two branches, and move towards releasing rgrass7 from this repo . The release may be the last, as rgrass will not stop supporting GRASS 7, but will not develop, I think.

@veroandreo
Copy link
Collaborator Author

Could someone add the CONTRIBUTING file to main, then I'll create the two branches, and move towards releasing rgrass7 from this repo . The release may be the last, as rgrass will not stop supporting GRASS 7, but will not develop, I think.

Indeed. We are really close to finally release GRASS 8. At the moment there's already RC2; meaning that 8 will soon become the stable version and 7 will only receive bug fixing but no new features. So, trying to get a working version of rgrass for GRASS 8 would be great :)

@rsbivand
Copy link
Owner

The 0.2-7 rgrass7 on the rgrass7 and main branches and the 0.3-1 rgrass on the rgrass branch both pass R CMD check --as-cran for both of 7.8.7RC1 and 8.0.0RC2 (locally built, Fedora 35). If soneone else would like to confirm, possibly on some other platform, then I suggest submitting rgrass7_0.2-7 soon. When 8.0.0 is released, this rgrass7 version will work. When 8* stabilizes, we can move to submit rgrass and once published, can add a suggestion to migrate to the rgrass7 startup mesage.

@VLucet
Copy link
Collaborator

VLucet commented Jan 23, 2022

Could someone add the CONTRIBUTING file to main

I've opened a PR for this see #38.
I can corfirm the package passes checks fine with 7.8 and, as far as I can tell, with 8.0.

I've also branched out of main to start setting up actions.

@florisvdh
Copy link
Collaborator

The rgrass7 branch (already merged in main) can now be updated to main (rebase or merge -ff) and pushed in order to keep both on par. The rgrass branch can receive the updates from main (or be rebased on it), and pushed. I'm proposing that also in order to double check there remains no misunderstanding about the workflows!

@florisvdh
Copy link
Collaborator

The 0.2-7 rgrass7 on the rgrass7 and main branches and the 0.3-1 rgrass on the rgrass branch both pass R CMD check --as-cran for both of 7.8.7RC1 and 8.0.0RC2 (locally built, Fedora 35).

@rsbivand I get following logs from R CMD check --as-cran under R 4.1.2, Linux Mint 20 (Ubuntu Focal):

  • logfile for current rgrass7 branch (361997c): some errors, one is an example, another is about PDF compilation (I assume), perhaps this is an effect of my local setup.
  • logfile for current rgrass branch (29bba4f): similar problems

On the command line I did R CMD build . followed by R CMD check <tarball> --as-cran. The result seems to be confirmed by rcmdcheck::rcmdcheck(args = "as-cran") in R, perhaps with slight differences in the presentation.

@rsbivand
Copy link
Owner

Most likely the setup. Please avoid all packages claiming to help, they only get in the way, this is probably where the crashes are coming from. The extra tarball shouldn't be there, something is making assumptions about which directory it thinks you are in.

From the directory above the git repo (in rgrass7 or rgrass branch), run R CMD build rgrass and check that the tarball name agrees. Then start GRASS (8.0.0RC2 or 7.8.7RC1) in that directory in nc_basic_spm_grass7, and inside GRASS run `R CMD check .tar.gz.

Latex is needed for manuals, if unavailable use --no-manual.

@rsbivand
Copy link
Owner

Is the double free or corrupt related to https://lists.osgeo.org/pipermail/grass-user/2022-January/082834.html ?

@florisvdh
Copy link
Collaborator

Thank you for the hints Roger. Also, it was interesting to learn compiling and installing GRASS 7.8.7RC1 and 8.0.0RC2 alongside the existing binary 7.8.6 installation from ubuntugis-unstable.

The check with rgrass7 + GRASS 7.8.7RC1 now runs fine here (previously I only had GRASS 7.8.6 from ubuntugis-unstable). Logfile (for now, used --no-manual) with 1 NOTE. The double free or corrupt is gone, and yes as hinted in the grass-user mail problems have arisen because rgdal wasn't yet recompiled with the recently updated GDAL/PROJ/GEOS from ubuntugis-unstable (with GRASS 7.8.7RC1, this also gave a segfault when crs was called in an example; after recompiling rgdal this was gone).

Same (no errors) for rgrass + GRASS 8.0.0RC2 - logfile with 3 NOTES.

Note to self (prompts and command for both cases):

GRASS 7.8.7RC1 (nc_basic_spm_grass7):/media/floris/DATA/git_repositories > R CMD check --as-cran --no-manual rgrass7_0.2-7.tar.gz

GRASS nc_basic_spm_grass7/PERMANENT:git_repositories > R CMD check --as-cran --no-manual rgrass_0.3-1.tar.gz

@rsbivand
Copy link
Owner

rgrass7_0.2-7 submitted to CRAN.

@rsbivand
Copy link
Owner

Source and some binaries (not yet all) rgrass7_0.2-7 on CRAN.

@florisvdh
Copy link
Collaborator

Hi @rsbivand we think the meeting minutes, like those of yesterday, will better fit in GitHub discussions than in GH issues. Could you activate the Discussions tab in the Settings of the repo?

Also I guess current issue can be closed since things have evolved quite a lot since Dec 2021.

@rsbivand
Copy link
Owner

rsbivand commented Jun 5, 2023

I opened discussions, I think. Its tinsel really irritated me. Can others open discussions, or only the repo owner?

@florisvdh
Copy link
Collaborator

Yes, it seems that opening a new discussion will work, at least I can, but probably anyone. Thanks.

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

No branches or pull requests

5 participants