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

modify FDB env var checks #414

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

cosunae
Copy link
Contributor

@cosunae cosunae commented Jul 5, 2024

This PR tries to solve two issues:

@iainrussell iainrussell added the approved-for-ci Approved to run CI on ECMWF machines label Jul 5, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (045d5d7) to head (162deb9).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #414   +/-   ##
========================================
  Coverage    90.10%   90.10%           
========================================
  Files          121      121           
  Lines         7905     7905           
  Branches       679      679           
========================================
  Hits          7123     7123           
  Misses         651      651           
  Partials       131      131           

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

fdb_home = os.environ.get("FDB_HOME", None)
fdb_conf = os.environ.get("FDB5_CONFIG", None)
if fdb_home is None and fdb_conf is None:
fdb5_home = os.environ.get("FDB5_HOME", None)

Choose a reason for hiding this comment

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

FDB5_DIR can also be used to locate the library

@sandorkertesz
Copy link
Collaborator

sandorkertesz commented Jul 12, 2024

Hi @cosunae, thank you for this PR.

I agree with you that the checking for FDB support should be more consistent. However, please consider these points:

  1. pyfdb finds the fdb5 library by using findlibs:

     libName = findlibs.find("fdb5")

    This call tries to find it by applying the algorithm described at: https://github.com/ecmwf/findlibs/tree/develop. It checks many things e.g. env variables like FDB5_HOME, FDB5_DIR etc, but it also checks e.g. the conda env.

  2. The checks in sources/fdb.py are meant to test the availability of the FDB config rather than that of the library, since if the library is not available loading sources/fdb.py will raise an exception immediately (see atop of the file).

  3. The checks you propose are too strict because with this I need both FDB5_HOME and FDB5_CONFIG/FDB5_CONFIG_FILE to be defined.

     if fdb5_home is None or (fdb5_conf is None and fdb5_config_file is None):
                raise ValueError(

    However, at the moment I am able to use pyfdb in my conda env (containing the FDB5 lib) having only FDB_HOME (!) defined.

@github-actions github-actions bot removed the approved-for-ci Approved to run CI on ECMWF machines label Jul 24, 2024
@cosunae
Copy link
Contributor Author

cosunae commented Jul 24, 2024

Thanks @sandorkertesz
let me reply to your comments:

  1. yes, my understanding of the find_libs algorithm is that it will mainly check for FDB5_XXX instead of FDB_XXX, therefore the reason for this proposal. Is this assumption right ?
  2. understood, thanks for the clarification.
  3. I see, yes, if FDB5_HOME is enough we should not make it stricter. I changed it to have any of the variables defined, can you check now ?

@sandorkertesz
Copy link
Collaborator

3. I see, yes, if FDB5_HOME is enough we should not make it stricter. I changed it to have any of the variables defined, can you check now ?

The new check seems to be fine now! However, internally we do use FDB_HOME, so I suggest you add it back and check its existence together with the other 3. Then this PR can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants