Feature #21909
closedRefresh R SDKs based on 3.0 API changes
Files
Updated by Peter Amstutz 8 months ago
- Release set to 70
- Target version changed from Development 2024-07-03 sprint to Development 2024-08-07 sprint
Updated by Peter Amstutz 7 months ago
- Target version changed from Development 2024-08-07 sprint to Development 2024-07-24 sprint
Updated by Peter Amstutz 7 months ago
- Target version changed from Development 2024-07-24 sprint to Development 2024-08-07 sprint
Updated by Peter Amstutz 6 months ago
- Target version changed from Development 2024-08-07 sprint to Development 2024-08-28 sprint
Updated by Peter Amstutz 6 months ago
- Target version changed from Development 2024-08-28 sprint to Development 2024-09-11 sprint
Updated by Peter Amstutz 5 months ago
- Subject changed from Refresh Java and R SDKs based on 3.0 API changes to Refresh R SDKs based on 3.0 API changes
Updated by Brett Smith 5 months ago
- Status changed from New to In Progress
This ticket now has a deep relationship with #19929, because the update process generates code based on the discovery document, so updating the R SDK has gotten more involved because of the work done there.
Updated by Brett Smith 5 months ago
- Related to Feature #19929: Improve documentation in the discovery document added
Updated by Brett Smith 5 months ago
So here's the history of this ticket:
I wasn't really sure what it meant to "refresh" the R SDK. I started working on this ticket when both Peter and Tom were out, so I couldn't ask questions. I couldn't find any relevant documentation. I did find #13076, which describes how autoGenAPI.R
generates code from the discovery document. I go and look and yeah, it looks relevant. It looks a lot like discovery2pydoc.py
, except it generates implementation in addition to documentation. I run it. Spot-checking a couple method signatures it generates, yeah, this definitely seems like what I want.
I kept working on it. The changes from #19929 revealed that the script wasn't prepared to handle more involved descriptions in the discovery documentation, so I dealt with those. I rigged things up so that the R SDK could be kept up-to-date with the API automatically, rather than requiring a manual run and committing artifacts to Git. I fixed up some documentation test failures too—although I didn't really understand what changes I had made (either to the script or the discovery document) that would've caused them, but hey, what do I know about R.
With #19929 basically finished, I started getting ready for review, and that's when I noticed the method names changed between 2.7.4 and my branch. I didn't think I made any changes that would change method names. I dug through my Git history looking for it. Still can't find anything even when I'm specifically looking for it. ???
So I dig deeper in Git history, and that's when I find the culprit: de8cd4c6a56cac85dbcf7fe1fcb29abe98fe9f18. This commit basically rewrites the previously-generated Arvados.R
without any corresponding change to autoGenAPI.R
. No, there is not a relevant change in a nearby commit either (git log --stat sdk/R/R
). I assume this current version of Arvados.R
was made with editor macros or something like that.
So. What do we do from here?
Option #1 is I extend my branch to update autoGenAPI.R
to generate code that's API-compatible with the current Arvados.R
. I do not think this is a huge task but I do not think it is a small task either. It kind of depends on whether the changes to Arvados.R
introduced lots of special cases or anything like that that will be hard to regenerate programmatically. I will look into that.
Option #2 is we abandon any kind of code generation for the R SDK and I prepare a new branch that deletes the obsolete infrastructure and, I guess, implements this ticket by updating Arvados.R
by hand. We will continue to have to do this for the foreseeable future. I expect this will lead to lots of fire drills where we're preparing a release and then we have to slap together some quick fixes to get the R SDK tests to pass because they've fallen out of sync with the API.
I know which option I prefer but I also realize I've already sunk way more time into this ticket than anyone probably anticipated and someone should probably check me before I keep digging this hole.
Updated by Peter Amstutz 5 months ago
- Target version changed from Development 2024-09-11 sprint to Development 2024-09-25 sprint
Updated by Brett Smith 5 months ago
21909-r-sdk-3.0 @ 9f44cabe4bbac28106b3f2c702b6502d27d9b77b -
developer-run-tests: #4444 and
developer-run-tests-doc-sdk-java-R: #2229 (developer-run-tests is one commit behind, but it's purely in the R SDK, so running doc-sdk-java-R against it should be sufficient)
- All agreed upon points are implemented / addressed.
- Updated code generator to incorporate earlier changes from de8cd4c6a56cac85dbcf7fe1fcb29abe98fe9f18
- Added a build system to the R SDK, removed committed build artifacts
- Updated code generator to incorporate more information from the discovery document, including extended documentation after #19929
- Anything not implemented (discovered or discussed during work) has a follow-up story.
- N/A. I will point out, I have a lot of concerns about the API of the wrapper methods, and how they deal with certain corner cases. I think there's a lot to clean up here. I did a little bit already when it actually simplified the implemntation: mostly doing filtering through the Arvados API rather than native R. I consider adding any more error handling to be out of scope. This branch got too big already.
- Code is tested and passing, both automated and manual, what manual testing was done is described
- Tested all wrapper methods (and by extension some generated methods) with the attached script. Reviewed the generated documentation by hand.
- Documentation has been updated.
- Yes, both the R reference and some of the build instructions.
- Behaves appropriately at the intended scale (describe intended scale).
- No change in scale of the actual SDK. R SDK builds are now more involved but in exchange they automatically stay in sync with the Arvados API, which is a net win and something we already do for a lot of our other SDKs.
- Considered backwards and forwards compatibility issues between client and server.
- Specifically extended the code generator to maintain backwards compatibility with the SDK in current main.
- Follows our coding standards and GUI style guidelines.
- N/A (no R style guide)
Updated by Peter Amstutz 5 months ago
You should add yourself to the author list (man/ArvadosR.Rd and DESCRIPTION)
I did a little poking at the package build process and it looks like as long as R CMD build R
(to build the tar.gz package) does the same thing then the publishing should be unchanged.
This LGTM!
Updated by Brett Smith 5 months ago
- Status changed from In Progress to Resolved
Peter Amstutz wrote in #note-13:
I did a little poking at the package build process and it looks like as long as
R CMD build R
(to build the tar.gz package) does the same thing then the publishing should be unchanged.
The command is R CMD build <SOURCE_DIR>
so as long as that command is running from arvados/sdk
(and that seems likely based on previous documentation) it's functionally identical to the new make package
.