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

Fix for issue 104 #105

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

Fix for issue 104 #105

wants to merge 3 commits into from

Conversation

afisher1
Copy link
Member

fixes issue #104. To test you must not have python helics package installed in your system level python. Create a python virtual environment with the helics python package. Activate your virtual environment and run a python helics federate using the helics runner. if it runs successfully it used the active virtual environment.

helics/cli.py Outdated Show resolved Hide resolved
helics/cli.py Outdated Show resolved Hide resolved
helics/cli.py Outdated Show resolved Hide resolved
Copy link
Member

@nightlark nightlark left a comment

Choose a reason for hiding this comment

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

This hard-coded changing of "python" to sys.executable is problematic when using a helics runner/cli installed by pipx, and any other scenario where we want to run a "python" binary other than the one the helics runner itself is installed in -- which I can see causing other bug reports down the line that would result in needing to revert the proposed fix.

From reading python/cpython#86207, it sounds like this issue comes down to Windows having search path rules for binaries that are different than POSIX, behaving in an unintuitive way with the Python virtual environment redirection script.

Using https://docs.python.org/3/library/shutil.html#shutil.which to make the search behavior for the executable to run consistent across platforms seems a bit better conceptually, though still doesn't fully resolve the issue that at least for now when a user is using Python it is best to just fully specify what virtual environment is being used to make it clear what is happening (TODO: update documentation).

@afisher1
Copy link
Member Author

I agree that shutil.which is the better route thanks @nightlark. Can you take another look? I was unsure of how we implement error handling in cli.py. I'm unfamiliar with click. Not sure we should be handling raising errors through click or not.

helics/cli.py Outdated Show resolved Hide resolved
Co-authored-by: Ryan Mast <3969255+nightlark@users.noreply.github.com>
@afisher1 afisher1 self-assigned this Nov 12, 2024
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.

3 participants