fzd: force calculators=1 for R-function models instead of just warning - #5
Merged
Merged
Conversation
…arning Funz/fz#80 makes fzd(model=<Python function>, calculators=N>1) actually evaluate the model concurrently in a worker-thread pool, instead of always running sequentially regardless of calculators as it did before. That change makes the fz.R wrapper's previous behavior unsafe: when calculators > 1 was passed alongside an R function model, fzd() emitted a warning saying the value "has no effect" but still forwarded it unchanged to the fz (Python) module. Once the Python side honors calculators > 1, that would call the R function back into the R session (via reticulate) from a worker thread -- reticulate callbacks are only safe from the main thread, so this would crash the R session. fzd() now unconditionally resets calculators to 1L whenever model is an R function and a value other than 1 was requested, after emitting a warning that explains why (mentions the crash risk and the forcing, not just "no effect"). calculators=1 or omitted remains silent, as before. calculators is still forwarded unchanged for file-based (non-function) models. Updated roxygen docs (man/fzd.Rd regenerated via roxygen2::roxygenise()) and the calculators=4L "Direct function model" example (misleading now that it's forced back to 1L for R functions). Added tests/testthat/test-fzd-calculators-forced.R, mocking get_fz() via testthat::local_mocked_bindings() to verify: calculators is forced to 1 (with a warning) when > 1 is requested for an R-function model; no warning when omitted or already 1; invalid calculators values are rejected; and calculators is forwarded unchanged for file-based models. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
R CMD check --as-cran flags non-ASCII characters as a WARNING, which fails CI since the workflow uses error_on: warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion to Funz/fz#80, which makes
fzd(model=<Python function>, calculators=N>1)actually evaluate the model concurrently in a worker-thread pool on thefz(Python) side, instead of always running sequentially regardless ofcalculatorsas it did before.That change makes this package's previous behavior unsafe: when
calculators > 1was passed alongside an R function model,fzd()emitted a warning saying the value "has no effect" but still forwarded it unchanged to thefzPython module. Once the Python side honorscalculators > 1, that would call the R function back into the R session (viareticulate) from a worker thread —reticulatecallbacks are only safe from the main thread, so this would crash the R session.Change
fzd()now unconditionally resetscalculatorsto1Lwhenevermodelis an R function and a value other than1was requested, after emitting a warning that explains why (mentions the crash risk and the forcing, not just "no effect").calculators = 1or omitted remains silent, as before.calculatorsis still forwarded unchanged for file-based (non-function) models.Also:
man/fzd.Rdviaroxygen2::roxygenise().calculators = 4L"Direct function model"@examplessnippet to1L(misleading otherwise, now that it's forced back to1Lfor R functions).Testing
Added
tests/testthat/test-fzd-calculators-forced.R, mockingget_fz()viatestthat::local_mocked_bindings()(no live Python/reticulate needed) to verify:calculatorsis forced to1(with a warning matching"forced to 1") when> 1is requested for an R-function model.calculatorsis omitted or already1.calculatorsvalues (non-scalar, non-numeric) are still rejected with an error.calculatorsis forwarded unchanged for file-based (non-function) models.Full suite:
devtools::test()→ 46 passed, 2 pre-existing failures unrelated to this change (test-modelica-examples.R, reproduced identically onmainbefore this branch — see PR discussion), 1 skip.