Send resolved OpenMDAO options to the server - #81
Merged
Merged
Conversation
RemoteExplicitComponent and RemoteImplicitComponent sent the raw constructor keyword arguments to the server rather than the component's resolved options. Any value that reached the component by another route never reached the server: an OpenMDAO default, or an assignment such as comp.options['dimension'] = 10 after construction. The failure was silent, since the component reported the option as set while the server kept computing with whatever it had. The options are now read from comp.options, restricted to the names the server declared through GetAvailableOptions so unrelated OpenMDAO options are not forwarded, and transmitted from client_setup immediately before the remote Setup call. Options declared without a value are skipped, leaving the server on its own default. The send moved out of the constructor rather than being duplicated. It cannot see post-construction assignment, so once setup sends, the constructor call is a redundant RPC carrying a subset of the truth. Setup-time placement is also the only one that stays ahead of Setup by construction rather than by coincidence, which matters for #76.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
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.
Fixes #77.
Problem
RemoteExplicitComponentandRemoteImplicitComponentsent the raw constructor keyword arguments to the server, not the component's resolved options. Any option value that reached the component by another route never made it across:comp.options['dimension'] = 10.The failure was silent. The component reported the option as set, the server computed with a different one, and the results were wrong rather than erroneous.
Fix
A new
utils.send_options(comp)builds the payload fromcomp.options, restricted to the names incomp._client.options_listso unrelated OpenMDAO options are not forwarded. Options that are declared but never assigned are skipped —declare_optionsgives them no default, so reading one raises, and leaving them out is what lets the server keep its own default.client_setupcalls it as its first act, immediately beforerun_setup().On the re-send question
The issue asked whether a re-send belongs in
setup()as well. The send moved there rather than being duplicated, and the constructor no longer sends anything.Sending at construction cannot see post-construction assignment, so once setup sends, the constructor call is a redundant RPC that only ever carries a subset of the truth. Setup-time placement is also the only one that stays ahead of
Setupby construction rather than by coincidence, which matters once #76 lands andSetOptionsafterSetupis refused withFAILED_PRECONDITION.The cost: a server rejecting an option value now surfaces at
prob.setup()instead of at construction. Nothing in the call path between the two needs options to be set, so nothing else moves.Tests
tests/test_openmdao_utils.py— unit coverage forsend_options: resolved values, unset options skipped, undeclared names filtered, and an ordering assertion thatsend_optionsprecedesrun_setup.tests/test_openmdao_{explicit,implicit}_client.py— new...ComponentOptionsclasses driving the real OpenMDAO option machinery against a mocked client, covering constructor kwargs, post-construction assignment, and unset options. The two constructor tests that pinnedsend_options(kwargs)now assert nothing is sent during construction.tests/test_openmdao_integration.py—test_rosenbrock_option_set_after_constructionbuilds withdimension=2, assigns4, and checks the server's variable shape over real gRPC. It fails on the pre-fix code with(2,) != (4,).Full suite: 324 passed.
Docs and
CHANGELOG.mdupdated in the same commit.