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

Issue 0133 create new spyro version were every dependency not pip installable is optional #137

Conversation

Olender
Copy link
Collaborator

@Olender Olender commented Nov 18, 2024

PR Description: Make Non-Pip-Installable Dependencies Optional in spyro and update license information

Summary
This PR addresses Issue #133 by modifying the spyro package to make all dependencies that cannot be installed via pip optional. This change improves the flexibility of the library, allowing users to install and use spyro even when certain dependencies are unavailable or difficult to set up. It also updates the license from general to the open-source LGPL-v3. The optional dependencies are SeismicMesh and ROL (ROL was already set as optional before). SeismicMesh is pip installable, however some of its dependencies (especially the required version of CGAL) can require sudo access for updates in older ubuntu versions or installation HPC clusters.

Key Changes

  • Updated setup.py.
  • Modified import statements throughout the codebase to handle missing dependencies.
  • Added or updated tests to ensure spyro functions correctly without the optional dependencies, where applicable.
  • Modified license file.

Impact

  • Users can install spyro with a minimal setup and only add the dependencies required for their specific use case.
  • Improves portability and ease of installation for users in constrained environments, especially those using Total Energy's HPC infrastructure.

Testing

  • Verified spyro runs successfully without optional dependencies.
  • Manually tested optional features to ensure they work as expected when dependencies are installed.

Olender and others added 2 commits November 18, 2024 10:40
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.95%. Comparing base (437cf28) to head (498354a).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
spyro/meshing/meshing_functions.py 60.00% 4 Missing ⚠️
spyro/solvers/acoustic_wave.py 42.85% 4 Missing ⚠️
spyro/tools/velocity_smoother.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   84.34%   84.95%   +0.61%     
==========================================
  Files          59       59              
  Lines        3935     3949      +14     
==========================================
+ Hits         3319     3355      +36     
+ Misses        616      594      -22     

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


🚨 Try these New Features:

spyro/tools/velocity_smoother.py Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test will run MLT or spectral methods depending on the environment. In either case, some part of the code will not be tested. I think the test environment should have SeismicMesh installed and run the test for every method that is supported by spyro.

We may also have another environment or mock approach for testing the logic of SeismicMesh availability. However, I guess this is not the scope of this test in particular.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are absolutely right about the importance of running the test for both types of 2D elements, especially to ensure comprehensive coverage. The reason I initially tested only one of these was that this particular test takes a significant amount of time to complete in our runner. However, I agree that we should incorporate both and fixed this in the latest commit.

As for testing the logic of SeismicMesh availability, I did attempt to implement something using the unittest package and mocks, but I encountered some unexpected issues that I couldn't fully resolve at the time. I plan to revisit this. If you have any suggestions or best practices for handling this kind of dependency testing (e.g., using mocks or creating a lightweight testing environment), I would greatly appreciate your input

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case we can try reducing the memory requirements of the test instead of skipping it. For example, we can try increasing "gradient_sampling_frequency". I left a low (and probably inappropriate) value for this field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good idea. Once the demeter runner finishes the current jobs, I will SSH into it and try out some different sampling values to see if I can reduce memory requirements without compromising the test.

I am working from home because of the holiday. Therefore, I don't want to risk loosing SSH access to my personal workstation in the lab (that only has 16 GB RAM) by accident while testing this. Demeter has significant more RAM (48 GB) and can run this test as is without a problem.

test/test_velocity_smoother.py Show resolved Hide resolved
@Olender Olender merged commit 6f0b0dd into main Nov 20, 2024
4 of 5 checks passed
@Olender Olender deleted the issue_0133-create-new-spyro-version-were-every-dependency-not-pip-installable-is-optional branch November 20, 2024 17: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.

Create new spyro version were every dependency not pip-installable is optional
2 participants