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

Wrap fortran files import in try/except block. #562

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Apr 14, 2023

The rest of files that import fortran modules do this, so I think in this case it should also be done for consistency.

The rest of files that import fortran modules do this, so I think in this case it should also be done for consistency.
@zerothi
Copy link
Owner

zerothi commented Apr 14, 2023

You are right. But perhaps we should just force the need for the fortran modules? Wouldn't this be acceptable? I don't even know how to install sisl without these modules...

from ..siesta._siesta import siesta_sc_off
try:
from ..siesta._siesta import siesta_sc_off
except:

Check notice

Code scanning / CodeQL

Except block handles 'BaseException'

Except block directly handles BaseException.
from ..siesta._siesta import siesta_sc_off
try:
from ..siesta._siesta import siesta_sc_off
except:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.
@pfebrer
Copy link
Contributor Author

pfebrer commented Apr 14, 2023

In v0.13.0 I removed the fortran extensions from setup.py and with this extra patch I could compile sisl to web assembly and use it without problems.

I don't know, it might be useful to allow sisl to be used without the fortran modules, just because the fortran extensions are not so easy to compile as the cython ones, at least for now. Compiling to web assembly is an example, but Windows is also another example (perhaps conda could compile easily for windows without the fortran functionality). Since the fortran functionality is "secondary", moreso now that SIESTA starts to write outputs in netcdf format, I would say it's reasonable.

Perhaps the try/except blocks could be removed from the main sisl source but maintain a patch file that when applied allows sisl to be compiled and ran without fortran.

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #562 (0bb2932) into main (85435d4) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
- Coverage   87.31%   87.31%   -0.01%     
==========================================
  Files         374      374              
  Lines       47171    47174       +3     
==========================================
+ Hits        41187    41188       +1     
- Misses       5984     5986       +2     
Impacted Files Coverage Δ
sisl/io/tbtrans/delta.py 92.26% <50.00%> (-0.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zerothi
Copy link
Owner

zerothi commented Apr 14, 2023

In v0.13.0 I removed the fortran extensions from setup.py and with this extra patch I could compile sisl to web assembly and use it without problems.

I don't know, it might be useful to allow sisl to be used without the fortran modules, just because the fortran extensions are not so easy to compile as the cython ones, at least for now. Compiling to web assembly is an example, but Windows is also another example (perhaps conda could compile easily for windows without the fortran functionality). Since the fortran functionality is "secondary", moreso now that SIESTA starts to write outputs in netcdf format, I would say it's reasonable.

Perhaps the try/except blocks could be removed from the main sisl source but maintain a patch file that when applied allows sisl to be compiled and ran without fortran.

Ok, fine. Lets continue to have this possibility. This should be easier with the cmake stuff. Later on we may add this functionality to disable fortran sources.

@pfebrer
Copy link
Contributor Author

pfebrer commented Apr 14, 2023

What might be better than the try/except would be a placeholder module that can be imported and tells the user that there's no fortran functionality when sisl tries to use something from that module.

@zerothi zerothi merged commit 40821c5 into zerothi:main Apr 14, 2023
@zerothi
Copy link
Owner

zerothi commented Apr 17, 2023

With the current scikit-build-core transitioning you can now do:

python -m pip install --config-settings=cmake.define.WITH_FORTRAN=NO .

and it will install everything, without the fortran sources compiled! :)

@pfebrer
Copy link
Contributor Author

pfebrer commented Apr 17, 2023

That is very cool! CMAKE_ARGS="-DWITH_FORTRAN=NO" python -m pip install should also work, right?

I've seen other packages prefix their specific flags, do you think it could make sense to name it SISL_WITH_FORTRAN to avoid interfering with other flags?

@zerothi
Copy link
Owner

zerothi commented Apr 18, 2023

That is very cool! CMAKE_ARGS="-DWITH_FORTRAN=NO" python -m pip install should also work, right?

Yes, I guess so. :)

I've seen other packages prefix their specific flags, do you think it could make sense to name it SISL_WITH_FORTRAN to avoid interfering with other flags?

The main idea about package prefixes is if the package will be a sub-project in a larger collection of packages, sisl I wouldn't regard as an obvious sub-package, so I don't think we should do this.

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