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

feat: update CLI to use async connection to DB #3450

Merged
merged 19 commits into from
Jul 26, 2023

Conversation

gabrielmbmb
Copy link
Member

@gabrielmbmb gabrielmbmb commented Jul 25, 2023

Description

This PR updates the CLI, so an async connection to the DB is used in the commands instead of a sync one. As the rest of the parts of the application are using the async connection, the code for the sync connection has been removed.

The list of changes of this PR are:

  • Add AsyncTyper class which allows to register async command with its method async_command. It executes then executes the command using asyncio.run.
  • Add run function that allows to execute an async command.
  • Remove database_async_url property from Settings class. From now on, database_url should have an URL with an async driver (sqlite+aiosqlite:// or postgres+asyncpg://).
  • Update Alembic connection to create an async engine to run the migrations.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested

  • All the unit tests regarding the CLI are working as expected
  • All the commands have been executed locally and working as expected (using both python -m argilla users create ... and python -m argilla.tasks.users.create ...)
  • Migrations have been applied without errors in a local environment to both SQLite and PostgreSQL instances.

Checklist

  • 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 filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@gabrielmbmb gabrielmbmb changed the base branch from develop to main July 25, 2023 08:30
@gabrielmbmb gabrielmbmb changed the base branch from main to develop July 25, 2023 08:31
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Patch coverage: 94.33% and project coverage change: +0.13% 🎉

Comparison is base (af60012) 90.39% compared to head (6abc09a) 90.52%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3450      +/-   ##
===========================================
+ Coverage    90.39%   90.52%   +0.13%     
===========================================
  Files          248      248              
  Lines        13232    13246      +14     
===========================================
+ Hits         11961    11991      +30     
+ Misses        1271     1255      -16     
Files Changed Coverage Δ
src/argilla/tasks/training/__main__.py 31.57% <ø> (+1.57%) ⬆️
src/argilla/tasks/async_typer.py 80.76% <80.76%> (ø)
src/argilla/server/settings.py 83.00% <94.73%> (+5.58%) ⬆️
src/argilla/__main__.py 100.00% <100.00%> (ø)
src/argilla/server/apis/v1/handlers/responses.py 100.00% <100.00%> (ø)
src/argilla/server/database.py 83.33% <100.00%> (+8.33%) ⬆️
src/argilla/tasks/database/migrate.py 41.66% <100.00%> (+2.53%) ⬆️
src/argilla/tasks/database/revisions.py 30.00% <100.00%> (+3.68%) ⬆️
src/argilla/tasks/users/__main__.py 100.00% <100.00%> (ø)
src/argilla/tasks/users/create.py 90.00% <100.00%> (+9.56%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gabrielmbmb gabrielmbmb added area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints client labels Jul 25, 2023
@gabrielmbmb gabrielmbmb added this to the v1.14.0 milestone Jul 25, 2023
@gabrielmbmb gabrielmbmb changed the title Feature/update cli async feat: update CLI to use async connection to DB Jul 25, 2023
@gabrielmbmb gabrielmbmb marked this pull request as ready for review July 25, 2023 12:34
@gabrielmbmb gabrielmbmb self-assigned this Jul 26, 2023
@frascuchon frascuchon merged commit ef1eb16 into develop Jul 26, 2023
14 checks passed
@frascuchon frascuchon deleted the feature/update-cli-async branch July 26, 2023 15:03
leiyre pushed a commit that referenced this pull request Aug 1, 2023
…rgilla into feat/shortcuts-improvements

* 'feat/shortcuts-improvements' of github.com:argilla-io/argilla:
  feat: update CLI to use async connection to DB (#3450)
  feat: add more value validations for rating questions (#3452)
  ci: selective `runs-on` value for tests execution (#3455)
  feat: update `package.yml` triggers (#3422)
  fix: uncancellable CI jobs (#3458)
  chore: Fix `ruff` line length (#3459)
  [pre-commit.ci] pre-commit autoupdate (#3449)
  improvement: Better efficiency of Weak Labels when vectors exist (#3444)
  refactor: add `ArgillaDatasetMixin` and re-structure `argilla.feedback.schemas` (#3427)
  chore: Set release version
  fix: add missing `suggestion_type_enum` values (#3445)
  [pre-commit.ci] pre-commit autoupdate (#3380)
  docs: fix username in HF Spaces docs (#3432)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants