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

Simplifying dependencies #323

Merged
merged 5 commits into from Nov 3, 2022
Merged

Simplifying dependencies #323

merged 5 commits into from Nov 3, 2022

Conversation

nmerket
Copy link
Member

@nmerket nmerket commented Oct 28, 2022

Pull Request Description

Removes most of the dependency version pins so it'll be able to update more easily.

Checklist

Not all may apply

  • Code changes (must work)
  • Tests exercising your feature/bug fix (check coverage report on Checks -> BuildStockBatch Tests -> Artifacts)
  • Coverage has increased or at least not decreased. Update minimum_coverage in .github/workflows/ci.yml as necessary.
  • All other unit tests passing
  • Update validation for project config yaml file changes
  • Update existing documentation
  • Run a small batch run to make sure it all works (local is fine, unless an Eagle specific feature)
  • Add to the changelog_dev.rst file and propose migration text in the pull request

@github-actions
Copy link

github-actions bot commented Oct 28, 2022

File Coverage
All files 81%
base.py 81%
eagle.py 74%
exc.py 57%
localdocker.py 44%
postprocessing.py 84%
utils.py 96%
sampler/base.py 69%
sampler/downselect.py 33%
sampler/precomputed.py 93%
sampler/residential_quota.py 50%
test/test_validation.py 96%
workflow_generator/base.py 90%
workflow_generator/commercial.py 53%
workflow_generator/residential.py 96%
workflow_generator/residential_hpxml.py 36%

Minimum allowed coverage is 24%

Generated by 🐒 cobertura-action against 04e7a02

Copy link
Member Author

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

See below for more specifics. In general, I'm allowing the latest versions of things to be installed again. Having so many dependencies was causing later versions of pip to run around in circles trying to find compatible combinations of dependencies.

'yamale>=2.0',
'ruamel.yaml>=0.15.0',
'testfixtures',
's3fs[boto3]',
Copy link
Member Author

Choose a reason for hiding this comment

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

We had to pin s3fs to a really old version so we could have boto3 and s3fs coexist. Turns out we weren't the only ones with this problem so they made a way to resolve the conflict, which we're using now. This allows us to use a newer version of s3fs among other things.

which pip
if [ $DEV -eq 1 ]
then
pip install --ignore-installed -e .
pip install --no-cache-dir --ignore-installed -e .
Copy link
Member Author

Choose a reason for hiding this comment

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

Keeps us from filling up a user's home directory with cached pip wheels. IYKYK.

conda create -y --prefix "$MY_CONDA_PREFIX" -c conda-forge "pyarrow>=7.0.0" "python=3.9" "numpy>=1.20.0" "pandas>=1.0.0,!=1.0.4" "dask>=2022.10.0" "distributed>=2021.5" ruby
source deactivate
source activate "$MY_CONDA_PREFIX"
mamba create -y --prefix "$MY_CONDA_PREFIX" -c conda-forge "python=3.10" "pyarrow" "numpy" "pandas" "dask>=2022.10.0" "distributed" "ruby"
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out they have mamba now, which makes things a lot faster.

Comment on lines +30 to +31
conda deactivate
conda activate "$MY_CONDA_PREFIX"
Copy link
Member Author

Choose a reason for hiding this comment

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

Swtiching to conda deactivate which we were getting warnings about.

@nmerket nmerket marked this pull request as ready for review November 1, 2022 20:33
@nmerket nmerket mentioned this pull request Nov 2, 2022
8 tasks
@nmerket nmerket requested a review from rajeee November 2, 2022 20:54
Copy link
Contributor

@rajeee rajeee left a comment

Choose a reason for hiding this comment

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

Thanks @nmerket. Looks good to me!

@nmerket nmerket merged commit 8bb4e79 into develop Nov 3, 2022
@nmerket nmerket deleted the deps-simplify branch November 3, 2022 16:11
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

2 participants