wandb / client-ng

Experimental wandb CLI and Python API - See Experimental section below.
http://wandb.com
16 stars 7 forks source link

Add priority checks to Settings class #157

Closed raubitsj closed 4 years ago

raubitsj commented 4 years ago

New rules:

TODO:

Future:

I think this PR has gotten big enough.. I think we should get this base in as it implements the fundamentals and is mostly accurate though until all the users of settings are updated, there might be some incorrect operations.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 6610962a-8d4f-41f8-a681-5dddefc03fd2


Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/wandb_settings_test.py 70 71 98.59%
wandb/sdk/wandb_login.py 2 3 66.67%
wandb/sdk/wandb_settings.py 187 211 88.63%
<!-- Total: 279 305 91.48% -->
Files with Coverage Reduction New Missed Lines %
wandb/backend/backend.py 1 88.71%
wandb/lib/git.py 1 68.0%
wandb/compat/tempfile.py 2 59.09%
wandb/lib/server.py 2 80.0%
wandb/sdk/wandb_settings.py 22 80.83%
<!-- Total: 28 -->
Totals Coverage Status
Change from base Build 6fd6741e-dcd6-4229-96b7-2f2fb83c2e99: 0.2%
Covered Lines: 12346
Relevant Lines: 20323

💛 - Coveralls
davidwallacejackson commented 4 years ago

So this seems like it'll work for implementing overrides and priority. Extending it to deal with dependencies seems a little messy, though. Concretely:

It's possible that we don't actually want to support most of that behavior -- it's kind of ridiculous -- but right now I'm having trouble drawing a hard distinction, based on the code, between what's supported and what's not.

One less-elaborate scheme I considered was designating certain settings as like... "primary" or "core" settings. Those settings would be un-overridable and could only be loaded from local sources (workspace, env, etc.), and would likely consist of things like API_KEY, PROJECT, and USERNAME. Then settings loading is effectively divided into two steps:

  1. load_primary, where we go through the local sources and resolve the canonical values for the primary settings only
  2. load_all, where we load the cloud sources based on the canonical primary values, and resolve the rest of the settings

This is still a little fraught, as primary settings can interfere with each other (env setting project, etc.) but probably reduces the number of plausible invalidation cases to a point where we can hardcode them.

Or maybe I'm massively overthinking all this? At any rate, now you know what I was turning over in my head all weekend.

raubitsj commented 4 years ago

@davidwallacejackson I think what you proposed with loading sources would work well and possibly simplify the code but it doesnt really fit in with how the code currently operates (we dont ever re-read or re-query sources when a parameter changes). Settings objects are used throughout the code as incremental objects. The settings evolve over the execution of the program when users use a new method with arguments to that method. Because of this nature of how we use settings we should optimize the incremental update.

This PR implements priority of sources, override priorities, and a subset of the dependency issue by requiring new settings properties which are purely computed.

raubitsj commented 4 years ago

As a bonus, I added a bunch more typing. more to come.