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

Implement and verify data model #35

Conversation

robert-cronin
Copy link
Contributor

Fixes #7

@robert-cronin
Copy link
Contributor Author

I've just added a draft for now including changes so far, I have a couple questions in mind:

  • I've noticed there is both permissions and roles, it seems roles is newer. Are we settling on something like role based permissions? if so maybe we can add roles table with permissions to make this extensible in the future? unless there is a clear idea of what roles/permissions will be in which case I guess it makes sense to hardcode it instead
  • It seems UserTeam (code) is doing the same thing as TeamUser (spec), I'll just use UserTeam going forward I guess?

cc @fearnworks

@fearnworks
Copy link
Collaborator

fearnworks commented Aug 29, 2024

  1. I've noticed there is both permissions and roles, it seems roles is newer. Are we settling on something like role based permissions? if so maybe we can add roles table with permissions to make this extensible in the future? unless there is a clear idea of what roles/permissions will be in which case I guess it makes sense to hardcode it instead

RBAC seems fine, but will need to discuss with others first.

  1. It seems UserTeam (code) is doing the same thing as TeamUser (spec), I'll just use UserTeam going forward I guess?

Yea, likely just need to update in the spec doc.

@robert-cronin
Copy link
Contributor Author

  1. I've noticed there is both permissions and roles, it seems roles is newer. Are we settling on something like role based permissions? if so maybe we can add roles table with permissions to make this extensible in the future? unless there is a clear idea of what roles/permissions will be in which case I guess it makes sense to hardcode it instead

RBAC seems fine, but will need to discuss with others first.

  1. It seems UserTeam (code) is doing the same thing as TeamUser (spec), I'll just use UserTeam going forward I guess?

Yea, likely just need to update in the spec doc.

I guess it makes sense for me to update the spec doc as I go, I'll add it to these changes!

Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
@robert-cronin robert-cronin marked this pull request as ready for review September 5, 2024 10:43
@robert-cronin
Copy link
Contributor Author

Some noteworthy changes for reviewers:

  • Enums are now defined in a central enums.py file, not sure why there were duplicate definitions, but let me know if we need those.
  • Some of the updated_at fields weren't being initialized on create
  • Parametrised the embedding dimensions in config.py
  • Improved organization of tests
  • There is the beginnings of better file organization in modules/odr_core/odr_core/crud/annotation, I think its a great idea but this PR is already quite large so I'll leave that for a later PR

Thank you in advance for your review!

@robert-cronin
Copy link
Contributor Author

robert-cronin commented Sep 5, 2024

Below is the code coverage report as I don't think we have a pipeline for that yet. I am getting 86% so far for the odr_core, I think its 80+% is a good target to aim for but let me know if we need to aim higher!

---------- coverage: platform linux, python 3.12.5-final-0 -----------
Name                                            Stmts   Miss  Cover   Missing
-----------------------------------------------------------------------------
odr_core/__init__.py                                0      0   100%
odr_core/config.py                                 32      0   100%
odr_core/crud/__init__.py                           0      0   100%
odr_core/crud/annotation/__init__.py                4      0   100%
odr_core/crud/annotation/annotation.py             43     29    33%   13-35, 39, 43, 52-69, 73-79, 85, 97, 109
odr_core/crud/annotation/annotation_rating.py      36      3    92%   34, 58, 75
odr_core/crud/annotation/annotation_report.py      29      1    97%   39
odr_core/crud/annotation/annotation_source.py      37      2    95%   66, 86
odr_core/crud/content.py                          123     42    66%   44, 71, 122, 132, 138-143, 149-163, 176, 180-184, 188, 192, 196-203, 207-212, 237
odr_core/crud/content_event.py                     33      0   100%
odr_core/crud/content_report.py                    30      2    93%   20, 40
odr_core/crud/content_set.py                       48      6    88%   20, 40, 49-51, 63
odr_core/crud/embedding.py                        146     44    70%   37-38, 46-48, 104, 131, 134, 137, 158, 161, 164, 199-209, 213-222, 229, 265-275, 279-288, 295, 312, 322, 328, 338
odr_core/crud/team.py                              45      0   100%
odr_core/crud/user.py                              84     35    58%   46, 51, 55, 71, 82-85, 94, 98-113, 117-122, 126-144
odr_core/database.py                               10      0   100%
odr_core/enums.py                                  38      0   100%
odr_core/models/__init__.py                         6      0   100%
odr_core/models/annotation.py                      62      0   100%
odr_core/models/base.py                             2      0   100%
odr_core/models/content.py                         95      0   100%
odr_core/models/embedding.py                       50      1    98%   44
odr_core/models/team.py                            22      0   100%
odr_core/models/user.py                            39      1    97%   64
odr_core/schemas/__init__.py                        0      0   100%
odr_core/schemas/annotation.py                     93      0   100%
odr_core/schemas/content.py                        95      0   100%
odr_core/schemas/content_report.py                 22      0   100%
odr_core/schemas/content_set.py                    19      0   100%
odr_core/schemas/embedding.py                      65      0   100%
odr_core/schemas/team.py                           21      0   100%
odr_core/schemas/user.py                           40      0   100%
odr_core/utils.py                                  34     34     0%   1-45
-----------------------------------------------------------------------------
TOTAL                                            1403    200    86%
Coverage XML written to file coverage.xml

================================================= 99 passed, 19 warnings in 84.70s (0:01:24) ==================================================

Signed-off-by: Robert Cronin <robert.cronin@uqconnect.edu.au>
@fearnworks
Copy link
Collaborator

80% is great! Will dig into this review today.

Copy link
Collaborator

@fearnworks fearnworks left a comment

Choose a reason for hiding this comment

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

Test suite and overall core modules are much cleaner. Great work!

@fearnworks
Copy link
Collaborator

Contributor

IRT the new module structure. Definitely agree with splitting these into submodules like you did with annotation going forward. Those initial ones were gettting too long in the tooth

@fearnworks fearnworks merged commit 12be86f into Open-Model-Initiative:main Sep 6, 2024
2 checks passed
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.

Verify Data Model
2 participants