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

Add Python function to get Fortran Version #41

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

akleb
Copy link
Contributor

@akleb akleb commented Apr 18, 2022

Purpose

The goal of this PR is to provide an example for how to insert the git version/commit hash into the compiled fortran object at build time. Then it is included in the python wrapper, so that a python script can check what version the compiled part of the code is.

This functionality is primarily for repostate. It would allow repostate to ensure that the compiled fortran version of the code is the same as the python version of the code. Before a run, repostate would check for a common function name: fortranVersion in the top level of a python module. If it exists, it will ensure that the version matches the version it found for the python package.

Expected time until merged

A week or two to discuss where to place this function to ensure that it can work with more difficult repos such as adflow that never import the fortran module unless the ADFLOW object is instantiated.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

I have tried it and it works, not sure how to implement tests

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@akleb akleb requested a review from a team as a code owner April 18, 2022 16:29
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #41 (cd2c4a5) into main (161cc2b) will increase coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   52.53%   52.56%   +0.02%     
==========================================
  Files           5        6       +1     
  Lines        1616     1621       +5     
==========================================
+ Hits          849      852       +3     
- Misses        767      769       +2     
Impacted Files Coverage Δ
pyspline/fortranVersion.py 50.00% <50.00%> (ø)
pyspline/__init__.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@akleb
Copy link
Contributor Author

akleb commented Apr 18, 2022

fortranVersion is not in the correct place right now. It should be available from the top level with from pyspline import fortranVersion. @nwu63 suggested we talk with @eirikurj before making this choice because he foresees this not being a straightforward choice with something like ADflow where the fortran module is not imported unless the ADFLOW object is instantiated. Does anyone have good ideas for this for repos like ADflow/IDWarp?

@akleb
Copy link
Contributor Author

akleb commented Apr 18, 2022

I tried to use preprocessor directives to compile the GIT_VERSION into the source with a -DGIT_VERSION=hash compiler flag, but what I know from C is not working here and I couldn't get an answer from the internet.

If someone knows how to do something like:

#ifndef GIT_VERSION
  val = 'NONE'
#else
  val = GIT_VERSION
#endif

We could track a version.f90 file that does something like that, probably making it cleaner than creating the file in the Makefile.

@joanibal
Copy link
Collaborator

I'm not going to be much help here.

It is ready for review as is?

@akleb
Copy link
Contributor Author

akleb commented Apr 19, 2022

This implementation is ready for review, but I think we should have a meeting with a larger group to discuss this feature and potential implementations before anything gets merged.

@bernardopacini bernardopacini removed their request for review September 22, 2022 13:17
@ewu63
Copy link
Collaborator

ewu63 commented Oct 10, 2022

I pushed a git-tracked implementation of version.f90. Alternatively, we could also have version.o built the same way as the other objects, if we change the file extension to .F90. But then we will probably have to move the -DGITVERSION stuff to FF90_FLAGS or something which I felt was less clean.

@ewu63 ewu63 requested review from eirikurj and sseraj October 11, 2022 03:45
@eirikurj
Copy link
Contributor

eirikurj commented Nov 7, 2022

Dont think this is robust enough solution. This solution works alright for cloning the repo. However, this will not work for users who download the released version of the code, as there is no git history. The version string will then be whatever error message is given.

I would, instead of the git version, use the version found in __init__.py. The lib is then consistent with the version there, which is the version reported you typically care about.

However, using this approach you need to run the code (import the lib at minimum) to get the version. How about using a more traditional workflow for shared libraries. Once the *.so has been built, then append the version number and use symbolic links. For example, libadflow.so -> libadflow.so.1.2.3, where libadflow.so.1.2.3 is the actual library. Again, the version here should be extracted from the __init__.py. The version is directly readable from the filename. Using this approach, then there is not really a need for embedding it in the library and some specialized code.

@ewu63
Copy link
Collaborator

ewu63 commented Nov 7, 2022

  • We can address the issue with releases by fetching the version from the init file, but only for releases
  • The main motivation of this work is for cases where you have made local, committed or uncommitted changes, then generated results with those. The init file is not going to provide sufficient information and that does not help much with regards to result reproducibility. Similarly for the so version numbering, it's fine for releases but this is meant more for developers making frequent updates to their codes. Having a git hash is essential, and just having the version string is insufficient.

@joanibal joanibal removed their request for review November 8, 2022 17:32
@sseraj sseraj removed their request for review June 14, 2024 15:17
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.

5 participants