Improve wandb.init(settings=) to handle Settings object similarly to dict parameter
#3510
Conversation
…d dict is treated
dmitryduev
requested review from
vanpelt,
raubitsj and
kptkin
and removed request for
vanpelt
Codecov Report
@@ Coverage Diff @@
## master #3510 +/- ##
==========================================
+ Coverage 81.41% 81.44% +0.03%
==========================================
Files 236 236
Lines 29082 29098 +16
==========================================
+ Hits 23677 23700 +23
+ Misses 5405 5398 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @pytest.mark.skip( | ||
| reason=( | ||
| "mock server enforces entity=mock_server_entity and project=test," | ||
| "this should only work on a live server, or mock_server would need customization." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This used to work bc previously, a Settings object passed to wandb.init() would "carry over" its setting sources. settings.update(...) by default uses Source.OVERRIDE meaning that no downstream changes would affect the changed settings (unless again overridden).
We could customize mock_server so that it "recognizes" two (or more) entities, but it feels a bit outside the scope for this PR.
| @@ -995,3 +995,13 @@ def test_local_api_key_validation(): | |||
|
|
|||
| # ensure that base_url is applied first without causing an error in api_key validation | |||
| wandb.Settings()._apply_settings(s) | |||
|
|
|||
|
|
|||
| def test_run_urls(): | |||
There was a problem hiding this comment.
This is a "replacement" test for the one in wandb_run_test.
| @@ -138,10 +141,23 @@ def setup(self, kwargs) -> None: # noqa: C901 | |||
| settings_param = kwargs.pop("settings", None) | |||
| if settings_param is not None: | |||
| if isinstance(settings_param, Settings): | |||
| # todo: check the logic here. this _only_ comes up in tests? | |||
There was a problem hiding this comment.
see, I even left a todo back in the day lol :)
| @@ -81,7 +82,7 @@ def _get_wandb_dir(root_dir: str) -> str: | |||
| return os.path.expanduser(path) | |||
|
|
|||
|
|
|||
| # fixme: should either return bool or error out. fix once confident. | |||
There was a problem hiding this comment.
fixme's break our flake checker, according to @raubitsj, so rm'ed 'em all.
| def items(self) -> ItemsView[str, Any]: | ||
| return self.make_static().items() | ||
|
|
||
| def get(self, key: str, default: Any = None) -> Any: | ||
| return self.make_static().get(key, default) |
There was a problem hiding this comment.
I thought, why not make the Settings object even more similar to a dictionary? It's pretty convenient in certain scenarios.
wandb/sdk/wandb_settings.py
Outdated
| @@ -917,11 +920,16 @@ def _run_url(self) -> str: | |||
| query = self._get_url_query_string() | |||
| return f"{project_url}/runs/{quote(self.run_id)}{query}" | |||
|
|
|||
| def _start_run(self) -> None: | |||
| def _start_run(self, source: int = Source.BASE) -> None: | |||
There was a problem hiding this comment.
nit: name seems weird given that we only time stamp this
There was a problem hiding this comment.
Agreed. I just kept the way it was called previously. How about _set_time_stamps?
There was a problem hiding this comment.
yeah makes more sense...
though it is weird that it is not part of some _apply_... something? not sure
There was a problem hiding this comment.
I'd keep this private method, but would be happy to move the call in wandb.init() to _apply_init. @raubitsj there's a todo in that place from you saying "# TODO(jhr): should this be moved? probably.", so I'd defer it to you to make the final decision thanks :)
Also, it's used in standalone_tests/grpc_client.py.
raubitsj
changed the title
Settings object passed to wandb.init() similar to how dict is treatedwandb.init(settings=) to handle Settings object similarly to dict parameter
Fixes #3505
Description
Treat a
wandb.Settingsobject passed towandb.init()similar to how a passeddictis treated. Will now detect settings that differ from defaults and apply them usingSource.INIT, as this is probably that the user would expect when doingwandb.init(settings=wandb.Settings(...)).Also deprecated
config_include_keysandconfig_exclude_keyskwargsinwandb.init(...)bc saw a todo in the code next to what I was touching and couldn't ignore it :)Testing
Hopefully fixed a couple tests + improved robustness in
tests/test_redir.pythat would flake out at times.