feat: add `User` class to manage and create users within Argilla #3169

alvarobartt · 2023-06-13T12:08:30Z

Description

This PR addresses the addition of a new user-management layer in the Python client via the User class, wrapping up the /api/users and /api/me endpoints in the API v0.

Closes #2968

Type of change

New feature (non-breaking change which adds functionality)
Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

Add unit tests for the new User class
Add unit tests for the user_api endpoints
Revisit existing tests to pass with updated UserModel (formerly named User)

Checklist

I have merged the original branch into my forked branch
I added relevant documentation
follows the style guidelines of this project
I did a self-review of my code
I made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

Improve `whoami` mock in `conftest`, and remove duplicated client-error tests

Aligned with API, as before there was no alignment due to the lack of an integration within the Python client

…r-management

Meant to be imported as `rg.User`

davidberenstein1957 · 2023-06-13T12:43:31Z

shouldn't we also be able to update users? or will we then need to delete and re-create?

alvarobartt · 2023-06-13T12:49:38Z

shouldn't we also be able to update users? or will we then need to delete and re-create?

That's a nice question indeed, maybe @frascuchon can answer this.

But for the moment, the only available endpoints for user-management are the following, so I guess we're not able to, but it's a nice addition for sure.

src/argilla/client/users.py

src/argilla/client/sdk/users/models.py

src/argilla/client/sdk/users/api.py

src/argilla/client/users.py

davidberenstein1957 · 2023-06-13T13:22:50Z

But for the moment, the only available endpoints for user management are the following, so I guess we're not able to, but it's a nice addition for sure.

Yes exactly, because I would expect to be able to update passwords, roles etc.

To be removed and replaced with the `UserModel` created upon #3169 completion

frascuchon · 2023-06-19T08:50:31Z

shouldn't we also be able to update users? or will we then need to delete and re-create?

That's a nice question indeed, maybe @frascuchon can answer this.

But for the moment, the only available endpoints for user-management are the following, so I guess we're not able to, but it's a nice addition for sure.

The CLI commands for users management are not using the API for the actions. I would prefer to provide basic support from python cli (by using available endpoints) and add more features later iteratively (which maybe means creating a new endpoint to update users, or reset passwords,...)

davidberenstein1957 · 2023-06-19T08:57:21Z

Perfect. @frascuchon could you potentially add some context here for our contributors?

To be removed once all the SDK functions are aligned to expect the `httpx.Client` to be provided instead of the `AuthenticatedClient`

src/argilla/client/sdk/users/models.py

src/argilla/client/users.py

src/argilla/client/sdk/users/api.py

src/argilla/client/users.py

frascuchon · 2023-06-20T13:56:13Z

tests/client/sdk/users/test_api.py

+def test_list_users(owner: User) -> None:
+    UserFactory.create(username="user_1")
+    UserFactory.create(username="user_2")
+    httpx_client = ArgillaSingleton.init(api_key=owner.api_key).http_client.httpx
+
+    response = list_users(client=httpx_client)


For this kind of tests I think you can use directly the client: TestClient, fixture

Suggested change

def test_list_users(owner: User) -> None:

UserFactory.create(username="user_1")

UserFactory.create(username="user_2")

httpx_client = ArgillaSingleton.init(api_key=owner.api_key).http_client.httpx

response = list_users(client=httpx_client)

def test_list_users(client: TestClient, owner: User, owner_auth_header: dict) -> None:

UserFactory.create(username="user_1")

UserFactory.create(username="user_2")

client.headers.update(owner_auth_header)

response = list_users(client=client)

But won't the ArgillaSingleton use the TestClient as configured in the conftest.py? Is this change worth it? Asking because if so, we should change this is more than one place, LMKWYT!

Yes, just to be more explicit here in tests if we're using the HTTP client directly. The conftest is a fixture to use the rg.init functions with an already mocked client

tests/client/test_users.py

Co-authored-by: Gabriel Martin <gabriel@argilla.io> Co-authored-by: Francisco Aranda <francis@argilla.io>

Also included under `TYPE_CHECKING` when applicable Co-authored-by: Francisco Aranda <francis@argilla.io>

src/argilla/client/sdk/users/models.py

codecov · 2023-06-20T14:35:04Z

Codecov Report

Patch coverage: 91.34% and project coverage change: +0.08 🎉

Comparison is base (51751ac) 90.91% compared to head (9a9d3d8) 90.99%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3169      +/-   ##
===========================================
+ Coverage    90.91%   90.99%   +0.08%     
===========================================
  Files          215      219       +4     
  Lines        11304    11663     +359     
===========================================
+ Hits         10277    10613     +336     
- Misses        1027     1050      +23

Flag	Coverage Δ
pytest	`90.99% <91.34%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/argilla/__init__.py	`86.66% <ø> (+3.33%)`	⬆️
src/argilla/client/datasets.py	`93.85% <ø> (ø)`
src/argilla/server/contexts/datasets.py	`96.01% <ø> (ø)`
src/argilla/server/seeds.py	`0.00% <ø> (ø)`
src/argilla/tasks/users/create.py	`91.11% <ø> (-4.45%)`	⬇️
src/argilla/client/apis/datasets.py	`91.72% <75.00%> (+1.35%)`	⬆️
src/argilla/client/feedback/dataset.py	`83.33% <75.00%> (+0.80%)`	⬆️
src/argilla/client/feedback/utils.py	`78.94% <78.57%> (+7.51%)`	⬆️
src/argilla/client/users.py	`84.40% <84.40%> (ø)`
src/argilla/client/workspaces.py	`86.31% <85.86%> (-7.44%)`	⬇️
... and 37 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/argilla/client/sdk/users/models.py

Co-authored-by: Francisco Aranda <francis@argilla.io>

#3729) # Description This PR addresses a pending TODO upon #3169 completion where the `UserModel` was included when wrapping the `/api/users` API endpoints. So on, in this PR we remove the `WorkspaceUserModel` schema in favour of `UserModel` to avoid duplicating the same schema to avoid potential incompatibilities. **Type of change** - [X] Refactor (change restructuring the codebase without changing functionality) - [X] Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** (Please describe the tests that you ran to verify your changes. And ideally, reference `tests`) - [X] Replace outdated references to `WorkspaceUserModel` with `UserModel` and re-run the existing unit/integration tests **Checklist** - [ ] I added relevant documentation - [X] follows the style guidelines of this project - [X] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [X] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

alvarobartt added 15 commits May 29, 2023 11:36

Curate docstring typos & redundancy

d6fd11f

Re-define whoami to expect a HttpxClient

4e5c129

Fix whoami unit tests

748eac8

Improve `whoami` mock in `conftest`, and remove duplicated client-error tests

Remove string-strip leftovers (" " )

344b958

Add UserModel and UserCreateModel

f0d02e3

Aligned with API, as before there was no alignment due to the lack of an integration within the Python client

Add list_users, create_user, and delete_user

c5033cb

Remove unnecessary type-hint

c54ffd4

Fix tests using outdated User & whoami

ead412b

feat(wip): add User & list_users

c707338

Merge branch 'develop' of github.com:argilla-io/argilla into feat/use…

4d5c7b9

…r-management

test: re-use mock_init_ok in test_aligned_argilla_versions

038a9e3

fix: rename ignore_none to exclude_none

2a04d42

fix: expect HTTP 200 in create_user

fc822ea

feat: add User class to manage existing users

e841f9f

feat: add User to __init__

852a3e2

Meant to be imported as `rg.User`

alvarobartt added this to the v1.11.0 milestone Jun 13, 2023

alvarobartt requested review from frascuchon and gabrielmbmb June 13, 2023 12:08

davidberenstein1957 reviewed Jun 13, 2023

View reviewed changes

alvarobartt added a commit that referenced this pull request Jun 13, 2023

feat: add temporary WorkspaceUserModel

e6c1ff4

To be removed and replaced with the `UserModel` created upon #3169 completion

frascuchon mentioned this pull request Jun 16, 2023

Update User management documentation #3212

Closed

davidberenstein1957 mentioned this pull request Jun 19, 2023

feature: extend CLI support for user management #3079

Closed

Merge branch 'develop' into feat/user-management

cd4d638

alvarobartt added 2 commits June 19, 2023 11:04

fix: avoid BaseModel.construct to keep validation

c3803c4

fix(test): wrong type-hint for argilla_user

b7ea3cd

alvarobartt added 3 commits June 20, 2023 14:01

feat: add httpx arg in __active_client

cd27927

To be removed once all the SDK functions are aligned to expect the `httpx.Client` to be provided instead of the `AuthenticatedClient`

fix: use UserRole enum instead of Literal

adf823a

test: add /api/users sdk unit tests

1627c93

alvarobartt marked this pull request as ready for review June 20, 2023 13:09

alvarobartt added 5 commits June 20, 2023 15:11

docs: update CHANGELOG.md

ba2e2f2

fix: delete_user arg is user_id not id

593b152

fix: exception handling in User

3c80f26

test: add User unit tests

ef2dd10

fix: remove outdated code-comment

409f19a