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

Fix CI mypy error: "WorkerFactory" has no attribute "app_config" #778

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

albertvillanova
Copy link
Member

@albertvillanova albertvillanova commented Feb 6, 2023

Fix type checking on WorkerLoop.loop method, when tryin to access the worker_factory's attribute app_config.

This PR fixes an issue introduced by:

src/datasets_based/worker_loop.py:100: error: "WorkerFactory" has no attribute "app_config"

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 6, 2023

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

@albertvillanova
Copy link
Member Author

Please note this is a quick fix so that our CI is working again.

We should think, as privately discussed with @severo, about the class hierarchy with the DatasetBased... intermediate level...

Maybe, we could use BaseWorkerFactory to define the interface, and rename DatasetBasedWorkerFactory to WorkerFactory...

@albertvillanova
Copy link
Member Author

The root error raised by mypy is that WorkerLoop.loop() is trying to get the attribute self.worker_factory.app_config on an instance of WorkerFactory, whereas the attribute app_config is defined only for its child DatasetBasedWorkerFactory(WorkerFactory).

@albertvillanova albertvillanova changed the title Fix type checking WorkerLoop.loop worker_factory Fix CI mypy error: "WorkerFactory" has no attribute "app_config" Feb 6, 2023
@albertvillanova
Copy link
Member Author

Another approach would be to define the attribute app_config in the interface WorkerFactory...

@severo
Copy link
Collaborator

severo commented Feb 7, 2023

The bug has been introduced by:

https://github.com/huggingface/datasets-server/pull/774/files#diff-4326852d55eab85b019c702a4579f696cb429aaf13586d8e3a1824581f8c9e34R100

@lhoestq I would vote to remove this log or move it elsewhere because we will soon implement #737, which will allow a worker loop to manage various endpoints.

@albertvillanova
Copy link
Member Author

albertvillanova commented Feb 7, 2023

I am making the corresponding changes and you can review them.

Revert "Fix test"

This reverts commit b59ef24.

Revert "Fix WorkerLoop.loop worker_factory"

This reverts commit a56e014.
@codecov-commenter
Copy link

Codecov Report

Base: 91.76% // Head: 91.80% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (0cc2bae) compared to base (2e920f6).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
+ Coverage   91.76%   91.80%   +0.04%     
==========================================
  Files          33       33              
  Lines        2246     2245       -1     
==========================================
  Hits         2061     2061              
+ Misses        185      184       -1     
Flag Coverage Δ
workers_datasets_based 91.80% <ø> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
...s/datasets_based/src/datasets_based/worker_loop.py 47.50% <ø> (+0.58%) ⬆️

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.

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

thanks - it's fine to remove it

Copy link
Collaborator

@severo severo left a comment

Choose a reason for hiding this comment

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

Thanks!!

@severo
Copy link
Collaborator

severo commented Feb 7, 2023

(I'm merging to fix the CI on other PRs)

@severo severo merged commit 91837d7 into huggingface:main Feb 7, 2023
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.

5 participants