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 ray test #87

Merged
merged 12 commits into from
Nov 23, 2021
Merged

Fix ray test #87

merged 12 commits into from
Nov 23, 2021

Conversation

TomTranter
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #87 (9251a1e) into main (6db7aa0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   97.38%   97.39%   +0.01%     
==========================================
  Files          21       21              
  Lines        1299     1307       +8     
==========================================
+ Hits         1265     1273       +8     
  Misses         34       34              

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Nov 23, 2021

Debugging GitHub Actions is weird 😅

Summary -

  1. For some reason, ray requires liionpack to be installed in the given environment as a library (I have no idea why).
  2. Now the workflows install liionpack using python setup.py install before running the tests.
  3. Same for the conda environment.
  4. Conda doesn't care about resolving conflicts between dependency versions while installing a library from source (setup.py).
  5. distributed requires dask==2021.10.0 and because of the above point, dask had to be pinned everywhere.

Edit: 4th and 5th point -> Use "dask[complete]"

@TomTranter
Copy link
Collaborator Author

Thanks for your work on this. I understand that when installing dask in conda the distributed package is included. I don't particularly like the idea of pinning dask so would we fix all our problems if we just didn't test the pip install?

@wigging
Copy link
Collaborator

wigging commented Nov 23, 2021

Do you need to explicitly provide distributed for the pip install? Running python -m pip install "dask[complete]" should install everything needed for Dask. More info at https://docs.dask.org/en/latest/install.html

@Saransh-cpp
Copy link
Member

Thanks, @wigging @TomTranter! Using "dask[complete]" worked!

environment.yml Outdated
@@ -27,7 +27,7 @@ dependencies:
- click
- autograd
- absl-py
- dask
- dask[complete]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is dask[complete] needed for the conda environment? Just using dask in the environment file works fine for me. I think dask[complete] is just needed for the pip install.

@TomTranter TomTranter merged commit 8c97257 into main Nov 23, 2021
@TomTranter TomTranter deleted the issue-86-fix-ray-test branch November 23, 2021 22:36
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.

None yet

3 participants