Skip to content

allow user to change llm - #23

Open
AlexNikic wants to merge 3 commits into
harmonydata:masterfrom
AlexNikic:develop
Open

allow user to change llm#23
AlexNikic wants to merge 3 commits into
harmonydata:masterfrom
AlexNikic:develop

Conversation

@AlexNikic

@AlexNikic AlexNikic commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

Lets the user choose which LLM Harmony uses for matching from R, in the same way they can from Python.

match_instruments() already accepted a model name through ..., but it always sent it to the API as framework = "huggingface", so the cloud-hosted models the Harmony API supports (OpenAI, Google, Azure OpenAI) were unreachable
from the R library. This PR promotes model to a documented argument and adds a framework argument alongside it, so
the full parameters object the API accepts can be set from R:

# a different Hugging Face model
match_instruments(instruments, model = "sentence-transformers/paraphrase-multilingual-mpnet-base-v2")

# a cloud-hosted model, on an API deployment that holds an OpenAI key
set_url("http://localhost:8000")
match_instruments(instruments, model = "text-embedding-3-large")

The framework is inferred from the model name, so switching LLM normally only needs model. An explicit framework
always wins, which keeps models the R library has not been taught about usable. parameters is now always sent,
populated with the API's own defaults, so the default behaviour is unchanged.

Also adds list_models(), which calls GET /info/list-models and returns a data frame of framework, model and
available. Availability is a property of the API deployment rather than of this package: the public API reports only
the three Hugging Face models as available, because the cloud models need API keys that the API holds.
list_models() is how a user finds out what the endpoint they are pointed at can actually serve.

Finally, failed API requests now raise an R error carrying the detail message returned by the API. Previously a
rejected model came back as a 422 whose body flowed on into the response handling and failed later on
conten$matches[[1]], which gave no clue that the model was the problem.

This is backwards compatible. model moves from ... to a named argument, so existing calls passing model = behave
identically; ... is retained. No new third party dependencies — httr::GET() and httr::status_code() come from
the httr package the library already imports.

Documentation: README gains a "Choosing which LLM to use" section covering list_models() and the Docker route to the
cloud models (docker run -p 8000:80 -e OPENAI_API_KEY=... harmonydata/harmonyapi plus set_url()), and
?match_instruments gains a "Choosing a model" section.

Fixes # 22

Type of change

  • New feature (non-breaking change which adds functionality)
  • Requires a documentation revision

Testing

New test file tests/testthat/test-model-selection.R. The framework-resolution tests run offline; the rest run
against the live API and are wrapped in skip_on_cran().

  • Framework is inferred correctly for each of the nine models the Harmony API knows about, across all four
    frameworks
  • An explicit framework overrides the inferred one, including for a model name the library does not know
  • An unknown model with no framework warns and falls back to huggingface, i.e. the previous behaviour
  • Invalid model and framework arguments are rejected with a clear error
  • list_models() returns the expected data frame, and reports the default model of match_instruments() as
    available
  • Matching with a non-default model (paraphrase-multilingual-mpnet-base-v2) returns a similarity matrix of the
    right shape and a diagonal of 1
  • Asking for a model the API does not have raises an informative error rather than failing later on the response
    shape
  • Existing crosswalk and negative-polarity tests still pass, confirming the default matching path is unchanged

To reproduce:

devtools::load_all(".")
testthat::test_dir("tests/testthat")   # 36 pass, 0 fail

and a full R CMD check returns Status: OK — no errors, warnings or notes.

The Harmony API and Python library are untouched by this PR, so the API unit tests are unaffected — the change only
alters what the R library puts in the request body, using fields the API already accepts (MatchParameters).

Test Configuration

https://api.harmonydata.ac.uk

Checklist

  • My PR is for one issue, rather than for multiple unrelated fixes.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules — N/A, no changes are needed in the
    Harmony API or Python library
  • I have checked my code and corrected any misspellings
  • I add third party dependencies only when necessary. — none added
  • If I introduced a new feature, I documented it — documented in the README, in ?match_instruments and
    ?list_models, and in NEWS.md. No script added to the script examples
    repository
    yet; happy to add one if that would be useful.

@AlexNikic

Copy link
Copy Markdown
Contributor Author

just quickly did this with claude opus 5 and it did pretty well

@AlexNikic

Copy link
Copy Markdown
Contributor Author

i also added a yaml that does an R CMD check on merge requests

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

Successfully merging this pull request may close these issues.

1 participant