gwmod / nlmod

Python package to build, run and visualize MODFLOW 6 groundwater models in the Netherlands.
https://nlmod.readthedocs.io
MIT License
34 stars 3 forks source link

get_exe_path is a bit noisy + question #359

Open dbrakenhoff opened 4 months ago

dbrakenhoff commented 4 months ago

I'm getting a lot of this message (shown below) in my script, every time I call nlmod.util.get_exe_path(). When I pass a version_tag, it still logs messages. Maybe DEBUG level logging is sufficient, or maybe this message isn't very interesting?

INFO:nlmod.util.get_flopy_bin_directories:The version of the executables will not be checked, because the `version_tag` is not passed to `get_flopy_bin_directories()`.

Also a question, I just want to use the modflow binaries downloaded using flopy, how do I get nlmod to find that file? The binaries are available on my path so typing mf6 in my terminal gets me the right mf6 version. From my environment (where nlmod does not have any binaries downloaded), nlmod.util.get_exe_path() is now preferring its own binary directory in different environments instead of the flopy directory that's included in my PATH environment variable. I thought maybe this had to do with editable installs in different environments, but it also found the binaries in an environment where I installed nlmod with pip install ..

Chances are fairly large that I've just messed up my environments too much, but I wanted to check here whether anyone else has observed similar behavior?

Steps to maybe reproduce:

  1. Download flopy binaries using get-modflow :flopy
  2. Add flopy bin directory to PATH
  3. Install nlmod in two Python environments
  4. Download binaries in one Python environment
  5. Activate the other Python environment.
  6. Start python and import nlmod to run nlmod.util.get_exe_path("mf6").
  7. Which mf6 is returned?
bdestombe commented 4 months ago

I'm getting a lot of this message (shown below) in my script, every time I call nlmod.util.get_exe_path(). When I pass a version_tag, it still logs messages. Maybe DEBUG level logging is sufficient, or maybe this message isn't very interesting?

INFO:nlmod.util.get_flopy_bin_directories:The version of the executables will not be checked, because the `version_tag` is not passed to `get_flopy_bin_directories()`.

Good point on lowering the debug level! Plus, maybe we should change a default value to get_exe_path(.., version_tag="latest").

Also a question, I just want to use the modflow binaries downloaded using flopy, how do I get nlmod to find that file? The binaries are available on my path so typing mf6 in my terminal gets me the right mf6 version. From my environment (where nlmod does not have any binaries downloaded), nlmod.util.get_exe_path() is now preferring its own binary directory in different environments instead of the flopy directory that's included in my PATH environment variable. I thought maybe this had to do with editable installs in different environments, but it also found the binaries in an environment where I installed nlmod with pip install ..

Chances are fairly large that I've just messed up my environments too much, but I wanted to check here whether anyone else has observed similar behavior?

Steps to maybe reproduce:

  1. Download flopy binaries using get-modflow :flopy
  2. Add flopy bin directory to PATH
  3. Install nlmod in two Python environments
  4. Download binaries in one Python environment
  5. Activate the other Python environment.
  6. Start python and import nlmod to run nlmod.util.get_exe_path("mf6").
  7. Which mf6 is returned?

Currently, the executable that is last installed on your system and matches the version_tag and repo(if version_tag is not None). It doesn't discriminate against different environments. Why did you perform step 4?

Thus since you explicitly downloaded the binaries again with step 4, I would presume the path of the last install in step 4. The executables installed during the first step would the the ones used if step 4 was not performed. If not, something is off. Could you have a look at this file: "flopy_metadata_fp = flopy_appdata_path / "get_modflow.json"? The last in line that matches version_tag and repo would be the path returned.

We could change this behavior of course.

bdestombe commented 4 months ago

PATH is not considered here

bdestombe commented 4 months ago

Wait, this was not completely true. The following conditional is there as well:

    if nlmod_bindir in flopy_bindirs:
        return nlmod_bindir

    if flopy_bindirs:
        # Get most recent directory
        return flopy_bindirs[-1]

    # Else download the executables
    if download_if_not_found:
dbrakenhoff commented 4 months ago

Alright, I understand it better now :).

Good point on lowering the debug level! Plus, maybe we should change a default value to get_exe_path(.., version_tag="latest").

This sounds logical to me, and is more explicit.

As for your question:

Why did you perform step 4?

I was curious whether there were new binaries available in a certain environment so I quickly used nlmod functions to download the latest ones for that environment. Then in the other environment I was running some models using the flopy binaries, but using nlmod.get_exe_path("mf6") pointed to the binaries from that first nlmod environment, which I did not expect.

It doesn't discriminate against different environments.

I thought environments had be respected also for the binaries, but I understand now it just plucks the most recent location from the flopy metadata JSON file (given no binaries were installed in the current active environment)? So the philosophy is more along the lines, "you asked for a specific version, so I'll give you one that matches, regardless of where it's located".

The one worry I have is that nlmod.util.get_exe_path() without a version tag (and no binaries installed in your current nlmod environment) just gives you the most recent mf6 that was installed. But I guess being explicit in the version you want, or always downloading binaries into your current environment is good modeling practice.

And if I want to override that behavior and find a specific binary, I can get nlmod.get_exe_path() to find my flopy binaries instead of any nlmod ones by passing a bindir.

bdestombe commented 4 months ago

The one worry I have is that nlmod.util.get_exe_path() without a version tag (and no binaries installed in your current nlmod environment) just gives you the most recent mf6 that was installed. But I guess being explicit in the version you want, or always downloading binaries into your current environment is good modeling practice.

What is you view on:

An additional question to all:

dbrakenhoff commented 4 months ago

I'm for defaulting to "latest" and documenting that it is a good idea to think about pinning mf6 versions for projects. I think pinning versions in nlmod will be a bit too annoying in development. It would be nice to be able to easily figure out what version_tag "latest" corresponds to, and/or which version of mf6 is contained within a specific version tag.

Instead of generating a path for the executable, I would like only the attributes version_tag and repo to be stored in the model_ds. The path to the executable+downloading happens when running the simulation. The keeps the to_modelds() command a bit leaner, only stores the information that affects the outcome. This would make model_ds a bit more transferrable.

I do like seeing the path in my model dataset, that gives me confidence that it's using the right binary to run the model. Maybe that's old fashioned thinking though? I'm fine with adding repo/version tag to make it more transferable though, as long as I don't have to type more :innocent: .

bdestombe commented 4 months ago

So that makes it 1 vs. 1 ;)

Team full path Team version_tag+repo
1 1

@OnnoEbbens, @rubencalje would you like to cast a vote?

I don't think we should do both as they can get out of sync.

dbrakenhoff commented 4 months ago

I'd be happy to switch my vote if nlmod.util.get_exe_path("mf6", ds=ds) gives me the path to an executable :innocent:

Also it would be nice to quickly extract the mf6 version from some version_tag: nlmod.util.get_mf6_version(version_tag="latest")?

dbrakenhoff commented 4 months ago

BTW, I lowered the log level to DEBUG in https://github.com/gwmod/nlmod/pull/360/commits/45c25be966c59fd124dfe7261e10db660a845023. Leaving this issue open for the other discussion on ds.attrs