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

[MRG] Dev extras #703

Merged
merged 15 commits into from
Jan 18, 2024
Merged

[MRG] Dev extras #703

merged 15 commits into from
Jan 18, 2024

Conversation

gtdang
Copy link
Collaborator

@gtdang gtdang commented Jan 12, 2024

This is a small PR to improve the classification & organization of extra dependencies. The key benefits to this change are:

  1. Streamlines developer dependencies install
    • Contributors only need to run pip install -e '.[dev]', to get all the dependencies for joblib, GUI, test, docs generation.
  2. Installation of dependencies for Github runners uses the dependencies specified by the setup file.
    • Prevents discrepancies between package versions specified in the setup file and workflow yml files
    • We don't have to remember to change dependency specifications in multiple files

Note:

  • The mpi4py was left out of the dependency group because it was causing issues in tests when added to the parallel group. So developers will still need to manually install mpi4py and openmpi as recommended in the contributor guide.

To do:

  • Test dev install with a fresh install on Mac and Windows

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d3dd255) 91.35% compared to head (5df2214) 91.35%.

❗ Current head 5df2214 differs from pull request most recent head 4657b5c. Consider uploading reports for the commit 4657b5c to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #703   +/-   ##
=======================================
  Coverage   91.35%   91.35%           
=======================================
  Files          25       25           
  Lines        4606     4606           
=======================================
  Hits         4208     4208           
  Misses        398      398           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntolley
Copy link
Contributor

ntolley commented Jan 12, 2024

@gtdang this looks great!

Before I forget, you should add this to whats_new.rst, as well as #696 (under bug fix?)

@@ -115,9 +115,13 @@ repository and use pip with the editable (``-e``) flag::

$ git clone https://github.com/jonescompneurolab/hnn-core
$ cd hnn-core
$ pip install -e '.[gui]'
$ pip install -e '.[dev]'
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is beautiful!

Copy link
Collaborator

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

Looks good. Don't you have to update circleci.yml too?

@jasmainak
Copy link
Collaborator

@gtdang you should make a branch to your own fork, not to the main repo. For now it's okay, just remember to delete dev-extras branch after the pull request is merged.

setup.py Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@gtdang
Copy link
Collaborator Author

gtdang commented Jan 16, 2024

@gtdang this looks great!

Before I forget, you should add this to whats_new.rst, as well as #696 (under bug fix?)

Thanks! Updated with the whats_new.rst with PRs #695 #696 and the current #703

@ntolley ntolley enabled auto-merge (rebase) January 17, 2024 18:19
@ntolley ntolley disabled auto-merge January 17, 2024 18:19
@@ -30,15 +30,13 @@ jobs:
conda create -n testenv --yes pip python=3.8
source activate testenv
conda install --yes scipy numpy matplotlib
pip install mne pooch tqdm psutil mpi4py joblib nibabel
pip install pooch tqdm mpi4py
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you still need to install pooch and tqdm separately?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't! Thanks for catching that!

@jasmainak jasmainak enabled auto-merge (rebase) January 18, 2024 15:23
@jasmainak
Copy link
Collaborator

@gtdang I enabled auto-merge. Don't forget to delete the dev-extras branch once you are done since it's on upstream

Thanks a lot for your contributions 🙏 🥳

@jasmainak jasmainak merged commit 4b7e2a0 into master Jan 18, 2024
15 of 16 checks passed
@gtdang gtdang changed the title [ENH] Dev extras [MRG] Dev extras Jan 18, 2024
@gtdang gtdang deleted the dev-extras branch January 18, 2024 20:56
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.

4 participants