-
Notifications
You must be signed in to change notification settings - Fork 26
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
🔧 dask compatible requirements listings #2420
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2420 +/- ##
======================================
Coverage 75.4% 75.5%
======================================
Files 560 560
Lines 20868 20868
Branches 2018 2018
======================================
+ Hits 15753 15763 +10
+ Misses 4580 4573 -7
+ Partials 535 532 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 just a minor change request
REQS_ENTRY_REGEX = re.compile(r"(\w+)==([\.\w]+)") | ||
NameVersionTuple = Tuple[str, str] | ||
|
||
def get_reqs(fname: str) -> Set[NameVersionTuple]: | ||
return set(REQS_ENTRY_REGEX.findall((requirements_folder / fname).read_text())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move these outside of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a special reason? I find it more clear inside of the only function it uses it. Short local functions are very common and useful: I can take advantage of e.g. the scope or shorter names . Generally, if I move these functions outside, i loose both: I would need to add more parameters and have longer names to avoid collisions and make it more clear in a wider context.
What do these changes do?
PROBLEM: The interaction between dask actors (i.e. client/scheduler/worker) demands a certain compatibility between libraries installed in different counterparts
SOLUTION: this PR provides listings for different type of clients under
services/dask-sidecar/requirements/_dask*.txt
that ensure compatibility (i.e. are in sync) with worker and scheduler services build withinservices/dask-sidecar
, i.e.services/dask-sidecar/requirements/_dask-complete.txt
services/dask-sidecar/requirements/_dask-distributed.txt
Other changes:
pip-compile
now strips extra labels from frozen requirements. (TODO: apply these changes to entire repo in a separate PR)pip install
CLI options to make it more readableRelated issue/s
provides minimal dask reqs for PR #2418
How to test
To test reqs compatibility
make devenv cd services/dask-sidecar make install-dev make test-dev-unit
and now can update requirements and retest