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.

Sign up for GitHub

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

Jump to bottom

Move workers/datasets_based to services/worker #800

Merged
merged 17 commits into from
Feb 13, 2023
Merged

Move workers/datasets_based to services/worker #800

merged 17 commits into from
Feb 13, 2023

Conversation

severo
Copy link
Collaborator

@severo severo commented Feb 9, 2023 •
edited
Loading

Based on #792.

@severo severo changed the base branch from main to check-dataset-connection-before-migration-job February 9, 2023 12:14
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023 •
edited
Loading

Codecov Report

Base: 90.66% // Head: 90.66% // No change to project coverage 👍

Coverage data is based on head (da0cc93) compared to base (6666942).
Patch coverage: 96.10% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #800   +/-   ##
=======================================
  Coverage   90.66%   90.66%           
=======================================
  Files         105      105           
  Lines        5292     5292           
=======================================
  Hits         4798     4798           
  Misses        494      494
Flag Coverage Δ
jobs_mongodb_migration 80.29% <ø> (ø)
libs_libcommon 92.82% <100.00%> (ø)
services_admin 86.84% <ø> (ø)
services_api 89.27% <ø> (ø)
services_worker 92.82% <96.08%> (?)
workers_datasets_based ?

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

Impacted Files Coverage Δ
services/worker/src/worker/asset.py 100.00% <ø> (ø)
services/worker/src/worker/resources.py 100.00% <ø> (ø)
services/worker/tests/constants.py 100.00% <ø> (ø)
services/worker/tests/fixtures/datasets.py 100.00% <ø> (ø)
services/worker/tests/fixtures/files.py 100.00% <ø> (ø)
services/worker/tests/fixtures/hub.py 98.01% <ø> (ø)
services/worker/src/worker/loop.py 50.00% <60.00%> (ø)
services/worker/tests/job_runners/test_splits.py 86.56% <76.92%> (ø)
services/worker/src/worker/job_runner_factory.py 92.30% <96.87%> (ø)
libs/libcommon/tests/test_queue.py 99.37% <100.00%> (ø)
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@HuggingFaceDocBuilder
Copy link
Collaborator

HuggingFaceDocBuilder commented Feb 9, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

@severo severo mentioned this pull request Feb 9, 2023
Merged
@severo severo force-pushed the check-dataset-connection-before-migration-job branch 4 times, most recently from 4c1d030 to ce2b651 Compare February 10, 2023 16:31
Base automatically changed from check-dataset-connection-before-migration-job to main February 10, 2023 17:20
severo and others added 14 commits February 10, 2023 17:32
@severo
feat: 🎸 use primitive parameters, add release, add tests
cf728f1
@severo
style: 💄 fix style
33c4f6a
@severo
feat: 🎸 use primitive parameters, add release, add tests
7c7ee79
@severo
style: 💄 fix style
b981e7a
@severo
log a warning when the migration cannot access database
027ca80 
thanks @AndreaFrancis
@severo @albertvillanova
Update libs/libcommon/tests/test_resources.py
dcaee42 
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
@severo
feat: 🎸 use primitive parameters, add release, add tests
59e57b4
@severo
style: 💄 fix style
cae10a8
@severo
feat: 🎸 move workers/datasets_based to services/workers
c0023a4
@severo
fix: 🐛 fix the Helm chart
bc9b2a8
@severo
feat: 🎸 upgrade the minor versions of the packages
ffb4a57 
and update the kenlm source
@severo
style: 💄 fix style
de7431a
@severo
test: 💍 fix the tests if the runner is slow
7bfc07d
@severo
fix: 🐛 refactor to avoid having worker.py in the root
547713c 
Having worker.py at the root is not allowed since it's also the name of
the package.
Now:
- WorkerLoop becomes Loop
- Worker becomes JobRunner

The terms are more accurate. Indeed, a JobRunner only processes one job.
@severo severo marked this pull request as ready for review February 10, 2023 17:39
AndreaFrancis
AndreaFrancis reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@AndreaFrancis AndreaFrancis left a comment

Choose a reason for hiding this comment

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

I put comments for minor changes. Just one question, how do we use that new vendor trec-car?

services/worker/pyproject.toml Outdated Show resolved Hide resolved
services/worker/src/worker/config.py Show resolved Hide resolved
@severo @AndreaFrancis
Update services/worker/pyproject.toml
5267f42 
Co-authored-by: Andrea Francis Soria Jimenez <andrea@huggingface.co>
@severo
Copy link
Collaborator Author

severo commented Feb 10, 2023

how do we use that new vendor trec-car?

it's not new, it's just moved with the rest of the code. It's a dependency required by some dataset scripts on the Hub:
https://github.com/huggingface/datasets-server/blob/5267f427dfa5bae2e4b4fba0b8042d4783fc3a39/services/worker/pyproject.toml#L39
I had to put it inside the code because it's not available in pypi or github or anything (as far as I could find).

severo and others added 2 commits February 10, 2023 21:09
@severo @AndreaFrancis
Update services/worker/src/worker/config.py
1bd9324 
Co-authored-by: Andrea Francis Soria Jimenez <andrea@huggingface.co>
@severo
Revert "Update services/worker/src/worker/config.py"
da0cc93 
This reverts commit 1bd9324.
@severo severo merged commit f43a0d2 into main Feb 13, 2023
@severo severo deleted the rename-worker branch February 13, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreaFrancis AndreaFrancis AndreaFrancis approved these changes

@lhoestq lhoestq Awaiting requested review from lhoestq

@albertvillanova albertvillanova Awaiting requested review from albertvillanova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@severo @codecov-commenter @HuggingFaceDocBuilder @AndreaFrancis