ETS-Next-Gen / writing_observer

Writing Observer and Learning Observer: A system for monitoring learning process data, with an initial focus on writing process data from Google Docs.
https://learning-observer.readthedocs.io/en/latest/
GNU Affero General Public License v3.0
11 stars 9 forks source link

WiP: PMSS integration #134

Closed bradley-erickson closed 6 months ago

bradley-erickson commented 7 months ago

This pull request is a work in progress for integrating PMSS into the Learning Observer platform.

bradley-erickson commented 7 months ago

Some settings in the system do a lot of checking to see if an object exists in the settings. Code similar to "google_oauth" in learning_observer.settings.settings['auth'] appears in a few different places and its not clear how we should handle that within LO or PSS. google_oauth is a key along a nested dictionary here.

A similar issue is handling dictionaries in general. Some pieces on LO expect a dictionary coming back from settings. We can easily change something like

# Currently looks like:
sub_dict = settings.settings['item_that_returns_dict']

# Could look like the following instead
sub_dict = {
    'val1': settings.settings.val1(types=['item_that_returns_dict']),
    # ... the rest of the dictionary items
}
# we build the dictionary in the code when we read in the settings

The most complex use case for settings I've encountered thus far is with the KVS. We have

KVS = KVSRouter(items=learning_observer.settings.settings['kvs'].items())

with settings that look like:

kvs:
  default:
    type: redis_ephemeral
    expiry: 86400
  memoization:
    type: redis_ephemeral
    expiry: 60
  ...

I'm not sure the best way to handle this use case. For now I'll leave it as is.

EDIT: The event_auth goes through a similar process of initializing itself.

    for auth_method in learning_observer.settings.settings['event_auth']:
bradley-erickson commented 7 months ago

Feature flags in the system seem a little messy at the moment. Some pieces of the code want to check if a certain item is available or not in the dictionary of feature flags. For fetching a specific setting, the code uses settings.feature_flag(flag). This code attempts to fetch the appropriate feature flag value.

Under the PSS integration, should we 1) modify the feature_flag method, so we can continue calling flag_setting = settings.feature_flag(flag) OR 2) manually call the items with pss flag_setting = settings.pss_settings.flag(types=['feature_flag'])

I think 1 is the correct option here. The modules should probably follow a similar route. At the time of writing this, I already changed the modules to use the 2 workflow (without initially thinking about 1).

pmitros commented 7 months ago

Feature flags in the system seem a little messy at the moment. Some pieces of the code want to check if a certain item is available or not in the dictionary of feature flags. For fetching a specific setting, the code uses settings.feature_flag(flag). This code attempts to fetch the appropriate feature flag value.

Under the PSS integration, should we

1. modify the `feature_flag` method, so we can continue calling
   `flag_setting = settings.feature_flag(flag)` OR

2. manually call the items with pss
   `flag_setting = settings.pss_settings.flag(types=['feature_flag'])`

I think 1 is the correct option here. The modules should probably follow a similar route. At the time of writing this, I already changed the modules to use the 2 workflow (without initially thinking about 1).

I would agree that we should stay with #1. There could be modified from pointing to creds.yaml to a thin wrapper around however we handle this in pss, and eventually, this might make it into pss as a first-class sort of thing (which might go together with lists and dictionaries more broadly).

pmitros commented 7 months ago

Some settings in the system do a lot of checking to see if an object exists in the settings. Code similar to "google_oauth" in learning_observer.settings.settings['auth'] appears in a few different places and its not clear how we should handle that within LO or PSS. google_oauth is a key along a nested dictionary here.

A similar issue is handling dictionaries in general. Some pieces on LO expect a dictionary coming back from settings. We can easily change something like

# Currently looks like:
sub_dict = settings.settings['item_that_returns_dict']

# Could look like the following instead
sub_dict = {
    'val1': settings.settings.val1(types=['item_that_returns_dict']),
    # ... the rest of the dictionary items
}
# we build the dictionary in the code when we read in the settings

The most complex use case for settings I've encountered thus far is with the KVS. We have

KVS = KVSRouter(items=learning_observer.settings.settings['kvs'].items())

with settings that look like:

kvs:
  default:
    type: redis_ephemeral
    expiry: 86400
  memoization:
    type: redis_ephemeral
    expiry: 60
  ...

I'm not sure the best way to handle this use case. For now I'll leave it as is.

EDIT: The event_auth goes through a similar process of initializing itself.

    for auth_method in learning_observer.settings.settings['event_auth']:

We had walked through several ways this might be handled, but IIRC, I think the strategy we converged on was to leave as-is, with list and dictionary-style settings continuing to live in creds.yaml, and moving over incrementally things which fit into pss to use pss, and finishing the complex pieces in a further PR.

pmitros commented 7 months ago

Notes:

We could do more than one of the above too.

bradley-erickson commented 7 months ago