Project

General

Profile

Actions

Feature #21909

closed

Refresh R SDKs based on 3.0 API changes

Added by Peter Amstutz 8 months ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Story points:
-
Release:
Release relationship:
Auto

Files

test30.R (1.64 KB) test30.R Brett Smith, 09/12/2024 08:36 PM

Subtasks 1 (0 open1 closed)

Task #22067: Review 21909-r-sdk-3.0ResolvedPeter Amstutz09/17/2024Actions

Related issues 1 (0 open1 closed)

Related to Arvados - Feature #19929: Improve documentation in the discovery documentResolvedBrett SmithActions
Actions #1

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
Actions #2

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2024-08-07 sprint to Development 2024-07-24 sprint
Actions #3

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2024-07-24 sprint to Development 2024-08-07 sprint
Actions #4

Updated by Peter Amstutz 6 months ago

  • Target version changed from Development 2024-08-07 sprint to Development 2024-08-28 sprint
Actions #5

Updated by Peter Amstutz 6 months ago

  • Target version changed from Development 2024-08-28 sprint to Development 2024-09-11 sprint
Actions #6

Updated by Peter Amstutz 5 months ago

  • Assigned To set to Brett Smith
Actions #7

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
Actions #8

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.

Actions #9

Updated by Brett Smith 5 months ago

  • Related to Feature #19929: Improve documentation in the discovery document added
Actions #10

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.

Actions #11

Updated by Peter Amstutz 5 months ago

  • Target version changed from Development 2024-09-11 sprint to Development 2024-09-25 sprint
Actions #12

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)
Actions #13

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!

Actions #14

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.

Actions

Also available in: Atom PDF