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

Update requirements #127

Merged
merged 20 commits into from
Oct 23, 2022
Merged

Update requirements #127

merged 20 commits into from
Oct 23, 2022

Conversation

jdeschamps
Copy link
Member

@jdeschamps jdeschamps commented Oct 6, 2022

I tested all notebooks with TF 2.7 and 2.10 (latest), and CSBDeep 0.7.2 (latest) in Python 3.9.

  • Lifted dependency requirement for CSBDeep in order to use recent TF versions
  • Fixed tests and made files creation happen in temporary folders
  • Solved deprecated warnings (tifffile.imsave, np.int)
  • Removed travis yaml
  • Added tox (tox.yml, .toml) files to run automated testing
  • Added github action workflow (to be tested), including deployment to PyPI

Here is roughly how I installed the environment:

conda install -c conda-forge cudatoolkit=11.2 cudnn
export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$CONDA_PREFIX/lib/
pip install tensorflow
pip install -e .

@jdeschamps jdeschamps requested a review from tibuch October 6, 2022 13:41
@jdeschamps
Copy link
Member Author

A lot of changes are also due to my IDE reformatting the file... But @tibuch maybe you could just have a look at 461505b because that is the only change to the code base.

Copy link
Collaborator

@tibuch tibuch 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 @jdeschamps for cleaning up!

So far I only glanced over it and it looks all reasonable to me. I will check it out and run the tests later today.

setup.py Outdated Show resolved Hide resolved
@jdeschamps
Copy link
Member Author

jdeschamps commented Oct 10, 2022

We also need to update the installation instructions. I will test this later today!

This page recommends to also specify cudnn version during installation.

@tibuch
Copy link
Collaborator

tibuch commented Oct 10, 2022

It is probably a good idea to follow this recommendation.

Could you ping me once you updated the instructions? Then I can test-run it from start to finish ✌️

@jdeschamps
Copy link
Member Author

jdeschamps commented Oct 10, 2022

It is probably a good idea to follow this recommendation.

Could you ping me once you updated the instructions? Then I can test-run it from start to finish v

Done (0c0f790).

I chose to follow the TF guidelines and link to it. We could also just say use tensorflow==2.7 or tensorflow==2.10 for safety.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tibuch
Copy link
Collaborator

tibuch commented Oct 11, 2022

Thank you @jdeschamps for all the work!

I was able to install everything and run the tests and example notebooks as they are.

I did not run the functional test. They just take too long.

I also tested the N2V2 functionality by setting

  • unet_residual = False
  • skip_skipone = True
  • blurpool = True
  • n2v_manipulator = 'median'

Training and prediction works well for the 2D case (in 3D the model is not initialized with an NotImplementedError #129). However, model.export does not work with the current blurpool implementation (see #128).

@jdeschamps
Copy link
Member Author

@tibuch oh good catch.

I have also not tried the functional tests, I have excluded them from tox testenv. I will start them on a virtual machine today and check them tomorrow.

One more thing. It seems that on my RedHat machine, I need to set the export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$CONDA_PREFIX/lib/ every time I start the environment new (e.g.: you close the terminal and restart it). Do you also have this or is it RedHat specific? It may be due to newer versions of TF...
If that is the case, that is super annoying and we should consider adding the export to the notebooks.

@tibuch
Copy link
Collaborator

tibuch commented Oct 11, 2022

This should be fixed by running this once in the acitvate env:

mkdir -p $CONDA_PREFIX/etc/conda/activate.d
echo 'export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$CONDA_PREFIX/lib/' > $CONDA_PREFIX/etc/conda/activate.d/env_vars.sh

@jdeschamps
Copy link
Member Author

This should be fixed by running this once in the acitvate env:

mkdir -p $CONDA_PREFIX/etc/conda/activate.d
echo 'export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$CONDA_PREFIX/lib/' > $CONDA_PREFIX/etc/conda/activate.d/env_vars.sh

I actually had missed your comments because I didn't refresh the page. Perfect!

@jdeschamps
Copy link
Member Author

The functional tests run through without errors btw.

FIxing exporting model weight
@jdeschamps
Copy link
Member Author

I merged here the fix for the model export (#128). However there is still the bug that we cannot save twice in a row. I believe this is due to the way CSBDeep saves the model to .pb format (see CSBDeep/CSBDeep#62).

Since the export works, I would suggest to create an issue about this bug to keep track of it and see what becomes of it in the next CSBDeep version. In the mean time since it does not prevent users from using N2V notebooks, I would go ahead with the new version release.

@jdeschamps jdeschamps merged commit 19fec3e into master Oct 23, 2022
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.

2 participants