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

Opencpu client dev #271

Draft
wants to merge 129 commits into
base: opencpu-client
Choose a base branch
from
Draft

Opencpu client dev #271

wants to merge 129 commits into from

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Dec 20, 2022

STATUS: NOT READY FOR REVIEW

Introducing a list_animal_ids() version that uses the OpenCPU API instead of local database access: list_animal_ids_api(), this function currently uses the optional json flag in the POST request and thus has to parse JSON to return a vector.

Dependencies

This pull request introduces a new dependency on httr, a wrapper of curl.

New helpers

  • get_credentials() fetches user credentials from the system environment, needed to access the api. These are then passed directly as strings in the HTTP request body. (needs review)
  • extract_temp_key() extracts the temp key from the path returned by a non-json POST request to the API.
  • get_val() takes the response object of a POST request to the api, and places a GET request for the evaluated response of the function. Uses RDS as an intermediary for compression.

The last two helpers are currently unused. I'm leaving these in for the moment in case we want to test compression for retrieval of bigger objects later on.

Branch Management

I'm planning to keep development on separate branches, and merge to OpenCPU-client, keeping it as up to date with main as possible.

Tests

Currently, the tests check the result of the API, not the API performance or if it's reachable (although if you can't reach it, you'll still fail the test).

What's next

  • Add check for get_credentials that credentials are set in the environment, instead of just returning empty strings.
    related to OpenCPU approach #242
  • Deploy etnservice (new API compliant version of get_acoustic_detections())
  • Develop get_acoustic_detections_api()
  • Select testing framework for the API (medium term)
  • Setup tests of API for uptime/performance (medium term)

TODO

following discussion in #280: I'm still using get_acoustic_detections() as a testcase.

  • API: better error handling in get_credentials()
  • use helpers for api or sql handling
  • remove get_acoustic_detections_api() from exports
  • write tests for api route sql route
  • wrap API code in generic helper
  • wrap main function code: if(api){out<-api_out}else{out<-sql_out} return(out) in generic wrapper
  • have helpers output to object that is handled in main function body
  • update get_acoustic_detections() documentation to reflect API Behaviour: authentication and switching between local db and API #280
  • update list_animal_ids() documentation
  • double check api fetching method is rda
  • adapt list_animal_ids() to use the helper structure from API Behaviour: authentication and switching between local db and API #280
  • pass error messages and warnings from api to client
  • set rate limiting for first step of api http request via helper OR use httr::RETRY() ref
  • set rate limiting for helper: get_val()
  • rewrite list_acoustic_project_codes()
  • rewrite list_acoustic_tag_ids()
  • rewrite list_animal_project_codes()
  • rewrite list_receiver_ids()
  • rewrite list_scientific_names()
  • rewrite list_station_names()
  • add assertation for api argument
  • close connections before returning output in _sql helpers
  • split off helpers from utils.R
  • [ ]

Testing effort

  • add tests for api helpers
  • achieve full coverage for api helpers
  • add tests for sql flow
  • achieve full coverage for api flow
  • skip sql tests when in CI

Documentation

  • create documentation for new api argument
  • Have functions inherit api argument documentation
  • add deprecation badge to connection argument
  • add deprecation documentation to connection argument
  • Have functions inherit connection argument documentation
  • port examples over to use api

@PietrH PietrH marked this pull request as draft December 20, 2022 09:53
@PietrH PietrH marked this pull request as ready for review December 20, 2022 10:07
@PietrH
Copy link
Member Author

PietrH commented Dec 20, 2022

We should probably merge main into opencpu-client as well

Copy link
Member

@peterdesmet peterdesmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Although the function can be named list_animal_ids_api() during development of v3, I would include it in list_animal_ids.R not in a separate file. I think that will make for a cleaner version history and will be easier to review params etc.
  • Place get_credentials() in a separate get_credentials.R file. utils.R should be reserved for internal functions
  • Rename opencpu-client branch to v3 as discussed
  • You mention functions currently not used. I would list those in a todo issue, so they don't linger in the code

R/list_animal_ids_api.R Outdated Show resolved Hide resolved
R/list_animal_ids_api.R Outdated Show resolved Hide resolved
R/list_animal_ids_api.R Outdated Show resolved Hide resolved
R/list_animal_ids_api.R Outdated Show resolved Hide resolved
R/list_animal_ids_api.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
tests/testthat/test-list_animal_ids_api.R Outdated Show resolved Hide resolved
@peterdesmet
Copy link
Member

I'm wondering if the current approach of naming new functions _api is a good one? Wouldn't it be better to introduce the new functions with their final name (at the top of their respective file) and rename the old ones to e.g. _db or _sql (pushed down in their current file until they are removed)?

The advantages I see are:

  • The functions that will be kept get their final name (no massive renaming at the end)
  • Test suite for the current SQL functions can immediately be used for the new functions (they are still used for the sql functions in the main branch)
  • No discrepancy between function name and file name (idem for test file names)

The only downside is that it isn't immediately clear which function now used the api and which one still used the DB, but I'm thinking an issue with a checklist could cover that?

PietrH and others added 30 commits June 16, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow client function to wait then retry after an error via purrr? A better error for a wrong password
2 participants