Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Implementation #1

Merged
merged 22 commits into from
Nov 4, 2024
Merged

Initial Implementation #1

merged 22 commits into from
Nov 4, 2024

Conversation

NeonDaniel
Copy link
Member

@NeonDaniel NeonDaniel commented Oct 30, 2024

Description

Define high-level structure of grouping models into (api, base, client, user, messagebus)
Consolidate existing models and add unit test coverage
Defines User Database schema and method for adapting to existing NeonCore schema
Defines base Message schema, including context
Defines __all__ in modules so import * can be used to import all Pydantic models from a module without also pulling extraneous imports (i.e. datetime which can cause confusion if both datetime and datetime.datetime are imported as datetime)

Issues

Other Notes

  • The intention is to also define all known Message schemas here, but this will require planning for how to organize - Messages as well as how much to enforce structure on what is currently a very unstructured system.
  • This module will in the future include the schemas used in HANA
  • Consider in the future defining schemas for MQ module communications (base schema included for now)
  • Ideally, the function sketched out in the util module will be extended to generate some browsable registry that may be included in documentation

Move existing unit tests to this repository
Refactor README.md into expected path
…obally

Document envvar to allow extra params
Remove bundled `schema` directory
Update `build_json_schema` util to be more flexible
…t sepc)

Update `node_v1` to extend base module and define `__all__`
Normalize newer `lat`/`lon` refs to `latitude`/`longitude` for consistency with database
Remove `verified` config options as those should not be self-reported
Remove unused `brands` config spec
Fix `UserProfile.from_user_object` bugs
Remove top-level import helpers
Add unit tests
…de other MQ message specs

Define Data schemas for all node messages
Move enum classes related to data schema to this module
Outline repo organization in README.md
Refactor node_v1 to improve `msg_type` validation, specify `data` models for all objects, and remove models that are used only in one other model
@NeonDaniel NeonDaniel marked this pull request as ready for review November 1, 2024 00:24
@NeonDaniel NeonDaniel requested a review from mikejgray November 1, 2024 00:24
neon_data_models/enum.py Outdated Show resolved Hide resolved
Comment on lines 73 to 83
class Weekdays(IntEnum):
"""
Defines weekdays.
"""
MON = 0
TUE = 1
WED = 2
THU = 3
FRI = 4
SAT = 5
SUN = 6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be replicating functionality available in the datetime library, but I'd have to check to be sure...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find it in searching.. I only find results for DayOfWeek in C and Java

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was thinking of datetime.isoweekday()...that's not really going to help in this scenario

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth moving to that eventually instead of this special enum, but that's more of an issue for the Alerts skill



class UserDbRequest(MQContext):
operation: Literal["create", "read", "update", "delete"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want to consider list to be another form of read or an entirely separate entity - I lean towards a separate entity myself but don't have strong opinions about it one way or another

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly.. right now there isn't any functionality for bulk actions, but that would be helpful in the future

Comment on lines +48 to +50
parser_data: dict
transcripts: List[str]
skills_recv: bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the description fields in KlatResponse - is there an intention to include those everywhere eventually?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, yes. It's just time consuming so I started with things that are ambiguous without them (describing dict keys, bcp-47 vs iso lang codes, etc)

username: str
data_to_remove: List[UserData]

msg_type: Literal["neon.clear_data"] = "neon.clear_data"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to annotate the entire class with a description? I've never run into this message type before and am now curious what it actually does

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe docstrings are rendered in swagger, but I'm not sure if that's technically part of the json schema

neon_data_models/models/user/database.py Outdated Show resolved Hide resolved
"""
Defines configuration used in BrainForge LLM applications.
"""
inference_access: Dict[str, Dict[str, List[str]]] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this default meets the type, technically? But I assume over time this will be more strictly defined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an empty dict technically satisfies the schema, but you are correct that this is still yet to be implemented/fully-defined

neon_data_models/models/user/database.py Outdated Show resolved Hide resolved
neon_data_models/models/user/neon_profile.py Outdated Show resolved Hide resolved
neon_data_models/models/user/neon_profile.py Outdated Show resolved Hide resolved
@NeonDaniel NeonDaniel requested a review from mikejgray November 4, 2024 17:50
@NeonDaniel NeonDaniel merged commit 638ddf2 into dev Nov 4, 2024
6 checks passed
@NeonDaniel NeonDaniel deleted the FEAT_InitialImplementation branch November 4, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants