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

Edit environment.yml + Add unit-tests for particle_metadata.ipynb #75

Merged
merged 11 commits into from
Jun 14, 2022

Conversation

ninamiolane
Copy link
Contributor

This PR:

  • cleans environment.yml by removing unnecessary github import of simSPI
  • adds test_notebooks.py file to test jupyter notebooks
  • tests particle_metadata.ipynb

Note that download_and_upload_with_osf.ipynb is not tested as the jupyter notebook is currently running for too long.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ninamiolane ninamiolane marked this pull request as draft May 4, 2022 16:40
@ninamiolane
Copy link
Contributor Author

There is a problem in this PR, as the unit-tests of ioSPI are taking over an hour, due to the installation of conda dependencies. (see screenshot below)

We should investigate the source of the problem, and add a fix so that the unit-tests do not take as long, because unit-tests that take over an hour will prevent many contributors from adding code to the library, as it will be very frustrating.

The conda dependencies are in the environment.yml file at the root of the repository. The long unit-tests have first been observed in PR #75 , but not in other recent PRs, thus it should come from additions made in this PR: the install of jupyter or the install of osfclient maybe?

Screen Shot 2022-05-09 at 2 16 07 PM

@ninamiolane
Copy link
Contributor Author

@artajmir3
Copy link
Collaborator

artajmir3 commented May 13, 2022

FYI, I've encountered the same issue in PR #83 exactly after the commit that added jupyter to enviroment.yml, and there is no osfclient in that PR. Hence, I think the issue is probably with the installation of Jupiter.

@ninamiolane
Copy link
Contributor Author

@artajmir3 thanks for the insight! Yes, we are not sure why this is taking so long. Let us know if you find out how to solve this jupyter issue? We need to solve it before we can merge this PR and your PR :)

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #75 (9513d5e) into master (0ee2cb5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files           5        5           
  Lines         223      223           
=======================================
  Hits          220      220           
  Misses          3        3           

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 0ee2cb5...9513d5e. Read the comment docs.

@bongjinkoo
Copy link
Collaborator

The same path problem occurred when installing jupyter with pip. Also, subprocess.check_call() didn't work. So I changed it to subprocess.run() and it works now. I think the DeppSource error can be ignored.

@bongjinkoo bongjinkoo marked this pull request as ready for review June 14, 2022 19:07
Copy link
Contributor Author

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Thank you @bongjinkoo !!

@ninamiolane ninamiolane merged commit da789df into master Jun 14, 2022
@ninamiolane ninamiolane deleted the nina-pdf branch June 14, 2022 19: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.

3 participants