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

product_version argument overridden by environment #2791

Closed
seanpearsonuk opened this issue May 7, 2024 · 8 comments · Fixed by #2888
Closed

product_version argument overridden by environment #2791

seanpearsonuk opened this issue May 7, 2024 · 8 comments · Fixed by #2888
Assignees
Labels
enhancement New feature or request. Feature work invariably requires testing-team involvement

Comments

@seanpearsonuk
Copy link
Collaborator

In solver = pf.launch_fluent(product_version=pf.FluentVersion.v241) the product_version is overridden by the PYFLUENT_FLUENT_ROOT environment variable, which should only hold in the absence of a specific version choice for the specific launch. In my opinion.

Please comment, @mkundu1 @prmukherj @hpohekar @raph-luc

@seanpearsonuk seanpearsonuk added the enhancement New feature or request. Feature work invariably requires testing-team involvement label May 7, 2024
@hpohekar
Copy link
Collaborator

hpohekar commented May 7, 2024

@seanpearsonuk

PYFLUENT_FLUENT_ROOT is a dev environment variable, user will not set it. So there will be only product_version if user specifies it otherwise FluentVersion.get_latest_installed() will be used.

image

@mkundu1
Copy link
Contributor

mkundu1 commented May 7, 2024

Right, the dev environments (like PYFLUENT_FLUENT_ROOT, PYFLUENT_LAUNCH_CONTAINER etc.) are not documented for users. They were added to run the same journal in different versions/platforms without any modification. These are useful in local development or CI.

@seanpearsonuk
Copy link
Collaborator Author

I realised that the environment variable is undocumented. I'm still querying whether the current behaviour is the most appropriate. If I set product_version in the context of a single call, it is inconvenient if it is not respected. I would not set the argument if I did not want to launch that version. My environment is permanently set up to point to a build but occasionally I want to run a different, specific version. I would have to override the environment as well as specifying product_version.

@raph-luc
Copy link
Member

raph-luc commented May 7, 2024

I think @seanpearsonuk raises a good point that the implementation of this environment variable may not be appropriate (even if it is only to be used for dev/CI). The environment variable should probably not override a launch_fluent argument specifying product_version. The env var should only come into play when product_version is unspecified (None).

On this topic, I would also vote for actually documenting all these environment variables somewhere in the docs, even if we don't expect users to use them. There will certainly always be niche use cases for these.

@mkundu1
Copy link
Contributor

mkundu1 commented May 7, 2024

Maybe I'm using the env var PYFLUENT_FLUENT_ROOT differently. This is what I usually do from a cmd. I also have PYFLUENT_FLUENT_ROOT pointed to a local build in system settings.

python -i <journal> # to run with the local build
set PYFLUENT_FLUENT_ROOT=
python -i <journal> # to run with the latest installed build (24.2)
set PYFLUENT_FLUENT_ROOT=%AWP_ROOTXXX%\fluent
python -i <journal> # to run with older installed builds
set PYFLUENT_FLUENT_ROOT=<build path>
python -i <journal> # to run with any other builds (e.g. daily build paths in linux)

The journal may contain product_version argument if it is coming from customer or recorded from Fluent, but I don't need to modify anything within the journal.

I'm open to modify the beahviour if that makes the usage simpler. I agree that we should document the dev environment variable usage.

@raph-luc
Copy link
Member

raph-luc commented May 7, 2024

The way you are using it explains why it was implemented this way. I've only personally used PYFLUENT_FLUENT_ROOT in situations where I was not providing a product_version launch argument (that argument was just unspecified).

If I wanted to run a journal, or interactively a command, that explicitly specifies product_version, I would intuitively expect PyFluent to respect what I am specifying with that argument at that moment, regardless of environment variables in the background. Does that make sense?

There isn't a right or wrong answer here in my understanding, just different use cases and whether you think that function arguments should be respected over environment variables that were set up. I do think it is a bit weird that currently we'd just ignore product_version completely. But I think it's fine if we want that env var specifically to test journals without having to make any changes to the journal.

@raph-luc
Copy link
Member

raph-luc commented May 7, 2024

Related question: is there another way to specify a local Fluent release similarly to PYFLUENT_FLUENT_ROOT but without using this env var? For example, if the user wanted to specifically launch a Fluent installed at /test/v242 rather than whatever the AWP_ROOT242 var points to? Would it even make sense for us to support a launch_fluent argument for that? In other words: a way to launch Fluent that doesn't rely on any env vars at all.

If, for example, we had a fluent_root argument for launch_fluent, that specifies the Fluent path (and necessarily overrides whatever product_version specifies), then I think we can start to see a pattern here and the env var usage becomes even clearer in my opinion.

Edit: then raising appropriate warnings if both arguments are specified (or the fluent_root env var and the product_version argument are both specified). Would make everything more intuitive I think. Let me know what your thoughts are, I think this would also address Sean's initial point.

@seanpearsonuk
Copy link
Collaborator Author

seanpearsonuk commented May 7, 2024

I think that if we are going to continue to ignore product_version then we should issue a warning. One of the issues for me was that I almost didn't notice.

@mkundu1 mkundu1 self-assigned this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Feature work invariably requires testing-team involvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants