compSPI / ioSPI

I/O and Data Visualization
MIT License
4 stars 7 forks source link

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

Closed ninamiolane closed 2 years ago

ninamiolane commented 2 years ago

This PR:

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

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

ninamiolane commented 2 years ago

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 commented 2 years ago

Installing jupyter (via pip) in geomstats' workflows does not take as long: https://github.com/geomstats/geomstats/blob/a7bb7e80802727312221e3d18f86bbb2c40a4825/.github/workflows/test.yml#L47

jupyter is in doc requirements: https://github.com/geomstats/geomstats/blob/a7bb7e80802727312221e3d18f86bbb2c40a4825/setup.cfg#L56

artajmir3 commented 2 years ago

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 commented 2 years ago

@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[bot] commented 2 years ago

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 commented 2 years ago

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.