Improve / fix naming in NL server #4263

shifucun · 2024-05-22T20:08:05Z

This PR aims to greatly disambiguate code logic by using distinct naming for various object. Currently there are 10+ names like embedding_xxx for class, object, dict and files with a lot of clashes.

With this change:

Use catalog to replace embeddings_config object and represent the all available (may not be used) model/index config
Use registry to replace embedddings_map object which holds all the model/client objects
Use env to replace embeddings_spec which is the additional config per environment
Make all the field naming the same across yaml file and dataclass object. This will help for parsing and error checking.
Use A and A_dict to differentiate dataclass object vs yaml dict.
Move attribute model part of registry

…to nl-env

pradh

Beautiful change! Thank you for breaking it up so that it is easy to review!

pradh · 2024-05-24T05:03:39Z

deploy/helm_charts/envs/worldbank.yaml

+    default_indexes:
+      - medium_ft
+    enabled_indexes:
+      - medium_ft


intend to remove others?

I intended to only keep medium_ft for these instances. For world bank, do you think we should keep sdg and undata?

Since worldbank is part of un, maybe keep it there, could come handy for any demos, etc.

pradh · 2024-05-24T05:05:14Z

nl_server/loader.py

 _LOCAL_ENV_VALUES_PATH: str = f'{Path(__file__).parent.parent}/deploy/helm_charts/envs/autopush.yaml'


 @dataclass
-class EmbeddingsSpec:
-  default_index: str
+class Env:


Add class comments?

pradh · 2024-05-24T05:05:55Z

nl_server/config.py

-EMBEDDINGS_SPEC_KEY: str = 'EMBEDDINGS_SPEC'
+REGISTRY_KEY: str = 'NL_REGISTRY'
+CATALOG_KEY: str = 'NL_CATALOG'
+ENV_KEY: str = 'NL_ENV'


A one-line comment on what each is?

pradh · 2024-05-24T05:07:09Z

nl_server/registry.py

@@ -33,57 +34,59 @@


 #
-# A map from specific index to embeddings stores and models.
+# A class to hold embeddings stores and models.


Is there a more precise definition, and distinguish it from Env and Catalog?

pradh · 2024-05-24T05:08:17Z

nl_server/config.py

-# Defines one embeddings index config.
-@dataclass
-class EmbeddingsConfig:
+# This represents the full catalog of models and indexes.


... a subset of them may not be enabled by the Env.

pradh · 2024-05-24T05:10:38Z

nl_server/routes.py

@@ -47,13 +48,12 @@ def healthz():
  if default_embeddings_loaded:
    return 'OK', 200

-  default_index_type = current_app.config[
-      config.EMBEDDINGS_SPEC_KEY].default_index
+  default_index_type = current_app.config[config.ENV_KEY].default_indexes[0]


[0] may not be safe? LHS has a check before access.

good point, updated the check.

shifucun · 2024-05-24T05:25:45Z

thanks for the review!

pradh · 2024-05-24T05:28:47Z

nl_server/routes.py

-    logging.warning('Health Check Failed: Default index name empty!')
+  nl_env = current_app.config[config.ENV_KEY]
+  if not nl_env.default_indexes:
+    logging.error('Health Check Failed: Default index name empty!')


There are warnings below, so change them all or not?

(original rational for warning was I thought this can happen while booting up, i think i saw this crash in custom dc - #4240)

Oh. Let me update here in next PR. I hope we don't need to make a real prediction call in /healthz then all logic here can be simplified.

ah, makes sense!

keyurva

Always a fan of using good names - thanks!

keyurva · 2024-05-24T05:45:02Z

tools/nl/embeddings/build_custom_dc_embeddings.py

@@ -142,9 +143,9 @@ def generate_embeddings_yaml(model_info: utils.ModelConfig,
      "version": 1,
      "indexes": {
          "custom_ft": {
-              "embeddings": embeddings_csv_handler.abspath(),
+              "embeddings_path": embeddings_csv_handler.abspath(),


It'll be cool if at some point, changes to the yaml format don't require changes to the custom embeddings tool.

I think update the generated yaml file should do that, so running code that generate v10 yaml file with _v10.yaml. and the server would look for _v10.yaml and not read _v9.yaml (which can not parse).

shifucun added 3 commits May 22, 2024 12:13

update nl server resource/object naming

ebd7aa2

Merge branch 'master' of https://github.com/datacommonsorg/website in…

d5b5d73

…to nl-env

Merge branch 'master' of https://github.com/datacommonsorg/website in…

3f96fdd

…to nl-env

shifucun requested a review from pradh May 22, 2024 20:08

shifucun changed the title ~~Improve / fix naming in NL server~~ Improve / fix naming in NL server [DO NOT MERGE BEFORE PROD RELEASE] May 22, 2024

shifucun added 5 commits May 22, 2024 13:18

Merge branch 'master' of https://github.com/datacommonsorg/website in…

976995c

…to nl-env

more fix

e2085ff

sync

e23a892

Merge branch 'master' of https://github.com/datacommonsorg/website in…

3406e63

…to nl-env

more

cd50fe6

shifucun changed the title ~~Improve / fix naming in NL server [DO NOT MERGE BEFORE PROD RELEASE]~~ Improve / fix naming in NL server May 23, 2024

shifucun requested a review from keyurva May 23, 2024 04:27

shifucun added 3 commits May 23, 2024 17:17

Merge branch 'master' of https://github.com/datacommonsorg/website in…

75f68a3

…to nl-env

add back model.yaml

39a4e71

fix tests

24f2f93

pradh approved these changes May 24, 2024

View reviewed changes

resolve comments

fd22fe5

shifucun enabled auto-merge (squash) May 24, 2024 05:25

wb

c1a7d6f

pradh approved these changes May 24, 2024

View reviewed changes

shifucun merged commit 32dc95b into datacommonsorg:master May 24, 2024
9 checks passed

keyurva approved these changes May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve / fix naming in NL server #4263

Improve / fix naming in NL server #4263

shifucun commented May 22, 2024 •

edited

Loading

pradh left a comment

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024 •

edited

Loading

shifucun May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

shifucun commented May 24, 2024

pradh May 24, 2024

shifucun May 24, 2024

pradh May 24, 2024

keyurva left a comment

keyurva May 24, 2024

shifucun May 24, 2024

Improve / fix naming in NL server #4263

Improve / fix naming in NL server #4263

Conversation

shifucun commented May 22, 2024 • edited Loading

pradh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradh May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shifucun commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyurva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shifucun commented May 22, 2024 •

edited

Loading

pradh May 24, 2024 •

edited

Loading