Tagging system for items - #1937
Conversation
ml-evs
left a comment
ml-evs
left a comment
There was a problem hiding this comment.
- fix e2e tests by making the test user an admin
- can we easily click on a tag and have it apply the filter?
- can we set up the global tags pre-defined at the server level?
Maybe future PR?
- group admin (and future project) admin level tags (future PR)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ml-evs/bump-pydantic-final-final #1937 +/- ##
====================================================================
+ Coverage 80.24% 80.74% +0.49%
====================================================================
Files 83 85 +2
Lines 7739 7986 +247
====================================================================
+ Hits 6210 6448 +238
- Misses 1529 1538 +9
🚀 New features to boost your workflow:
|
|
davidwaroquiers
left a comment
davidwaroquiers
left a comment
There was a problem hiding this comment.
I quickly reviewed the changes. Looks all good to me. Maybe the only thing could be to explicitly mention somewhere (maybe both in the code docstrings, e.g. in HasTags, and in the documentation, there is no doc about the tags for now, probably good to add when this has been reviewed by @datalab-org/datalab-dev )
| - `AccessScope.USER`: a user-defined tag owned by exactly one user; only that | ||
| user can list, use, edit and delete it. |
There was a problem hiding this comment.
Maybe good to clarify that any tag that is applied to an item or any other object that would have HasTags will be visible to any user who has access to that item. Not sure if this should be here, in HasTags, somewhere in the docs, or in multiple of these.
There was a problem hiding this comment.
I agree that it would be better to specify this in the docstrings. The natural place may be AccessScope description, but it is not obvious that this would be a general behaviour. Maybe HasTags?
There was a problem hiding this comment.
Let's indeed put it in HasTags, and then it can be added later in the user doc somewhere at some point.
| @@ -1,3 +1,4 @@ | |||
| # This file was edited with the assistance of an AI model and requires human review from the contributor. | |||
There was a problem hiding this comment.
Forgot to remove after review ;-)
|
@ml-evs we think this might be ready for your review. |
OMWalmsley
left a comment
OMWalmsley
left a comment
There was a problem hiding this comment.
This feature seems to be working on my machine. My comments are mostly about replacing the custom jsonify errors with werkzeug.exceptions, similar to what has been done previously. This should hopefully standardise things a bit more and reduce the line count slightly.
| try: | ||
| scope = AccessScope(data.get("scope") or AccessScope.USER.value) | ||
| except ValueError: | ||
| return ( |
There was a problem hiding this comment.
Lots of the routes have custom jsonify errors, these could be converted into werkzeug exceptions (https://werkzeug.palletsprojects.com/en/stable/exceptions/) as has been done in various other places in the code base.
There was a problem hiding this comment.
Thanks @OMWalmsley for this comment. We were wondering what should be used within datalab. We actually looked at how it's done and basically custom jsonify errors (~110) are more common in datalab than werkzeug errors (~50) so we went for the jsonify errors initially but I agree that it would be good to standardize all of this. We can of course convert all these custom jsonify errors to their corresponding werkzeug exceptions. More generally, maybe it would be good to open an issue to standardize all of these in the other routes as well ? @ml-evs may have some comment on this too ?
There was a problem hiding this comment.
Following today's meeting with @ml-evs and @OMWalmsley, the plan is to use werkzeug exceptions when it corresponds to a clear api route exception, but raise a proper and adequate error/exception when it is a runtime/datalab related error.
There was a problem hiding this comment.
I have replaced most of the jsonify exceptions in tags.py with werkzeug.
|
|
||
| if scope == AccessScope.GLOBAL: | ||
| if not _is_admin(): | ||
| return ( |
There was a problem hiding this comment.
Similar to previous comment (Werkzeug errors)
| if owner is None: | ||
| return ( | ||
| jsonify( | ||
| status="error", |
There was a problem hiding this comment.
And here and various other places I haven't listed
| if _name_conflict_exists(name, scope, owner): | ||
| return ( | ||
| jsonify(status="error", message=f"A tag named {name!r} already exists."), | ||
| 409, # 409: Conflict |
There was a problem hiding this comment.
Here (Werkzeug errors). For example: werkzeug.exceptions.Conflict
0969519 to
6d809c7
Compare
f112cf0 to
765d3b1
Compare
623e1da to
c075b7a
Compare
d41762c to
5929dfe
Compare
5929dfe to
f122121
Compare
4 participants
As anticipated in #1764, this PR introduces tags for annotating items with a dedicated management page and two permission scopes.
The feature is off by default, enabled through the
PYDATALAB_ENABLE_TAGSvariable.Note that the branch is based on the pydatinc2 one. The idea is that it will be merged in main after the pydantic2 branch is merged in main.
Here are the main features of the tags:
Scopes
Two scopes:
Notes:
personal tag; they may remove any tag from an item they can edit
(including another user's personal tag).
Data model
Tagentry in a newtagscollection.scopeandownerattributes. Explicitly decided not to useHasOwneras, after internal discussions, we considered that belonging to multiple creators or groups may open the road to all sort of accessibility issues. This seemed a different was of scoping the ownership. If preferred the same logic could probably be achieved usingHasOwnerand enforcing in the code that only one creator can be set, with no creator meaning global tag.tagsfield through theHasTagsmixinFrontend
/tagsmanagement page: create/edit/delete the tags.