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] Add resources folder to package data #772

Merged
merged 2 commits into from
Aug 19, 2021
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 16, 2021

Closes None. I was just testing a new install from the main branch and got the following:

Traceback (most recent call last):
  File "/home/data/nbc/misc-projects/Salo_PowerReplication/code/processing/04_run_multi-echo_denoising.py", line 255, in <module>
    run_tedana(project_dir, dset)
  File "/home/data/nbc/misc-projects/Salo_PowerReplication/code/processing/04_run_multi-echo_denoising.py", line 127, in run_tedana
    prefix=prefix,
  File "/home/data/nbc/misc-projects/Salo_PowerReplication/code/conda_env/lib/python3.7/site-packages/tedana/workflows/t2smap.py", line 220, in t2smap_workflow
    make_figures=False,
  File "/home/data/nbc/misc-projects/Salo_PowerReplication/code/conda_env/lib/python3.7/site-packages/tedana/io.py", line 84, in __init__
    config = load_json(config)
  File "/home/data/nbc/misc-projects/Salo_PowerReplication/code/conda_env/lib/python3.7/site-packages/tedana/io.py", line 308, in load_json
    with open(path, 'r') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/home/data/nbc/misc-projects/Salo_PowerReplication/code/conda_env/lib/python3.7/site-packages/tedana/resources/config/outputs.json'

It looks like we forgot to add the new json files to our package data so that they'll be bundled with the library in distributions.

Changes proposed in this pull request:

  • Add resources/config/* to package data in setup.py.

@tsalo tsalo added the bug issues describing a bug or error found in the project label Aug 16, 2021
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #772 (3ea3e9d) into main (3147e01) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #772   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files          27       27           
  Lines        2209     2209           
=======================================
  Hits         2059     2059           
  Misses        150      150           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3147e01...3ea3e9d. Read the comment docs.

@jbteves
Copy link
Collaborator

jbteves commented Aug 18, 2021

Intriguingly, I pulled main and it still executed the four-echo test just fine, so it's difficult for me to tell if this PR actually fixes the issue since I can't reproduce it. Question, though: why not use find instead of manually listing the directories in setup.py?

@tsalo
Copy link
Member Author

tsalo commented Aug 18, 2021

Intriguingly, I pulled main and it still executed the four-echo test just fine

When you say you pulled main, do you mean you used git? Or did you use pip? If you pulled the repository, then the data files will still be available.

To reproduce:

pip install git+https://github.com/ME-ICA/tedana.git

If you then run tedana on example data, it should raise the FileNotFoundError.

Question, though: why not use find instead of manually listing the directories in setup.py?

I was just focused on fixing the bug, rather than improving our packaging. If we want to change our packaging, I would probably use setup.cfg (like I do in NiMARE), based on Chris M's packaging recommendations: https://gist.github.com/effigies/9bbb424535d6a1d838d6325191c0a736

@jbteves
Copy link
Collaborator

jbteves commented Aug 18, 2021

Ah, I see. I didn't realize that was the failure mode; now reproduced. Okay, I think this will patch it. We may want to change packaging as part of our next release, but we can discuss tomorrow and there's no reason to hold this up. Thanks for the fix!

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @tsalo !

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

LGTM!

@tsalo
Copy link
Member Author

tsalo commented Aug 19, 2021

Thanks @jbteves and @eurunuela! Merging now.

@tsalo tsalo merged commit d4649ee into ME-ICA:main Aug 19, 2021
@tsalo tsalo deleted the fix-package-data branch August 19, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues describing a bug or error found in the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants