Skip to content

Conversation

@jeertmans
Copy link
Contributor

Description

A clear and concise description of what this pull request does.

  • Other contributions

Hello! While this wasn't any big issue, I think that using os.system("pip install sionna") is
generally a bad idea,
especially as it can be problematic for people with
multiple virtual environments or Jupyter kernels.

Indeed, calling pip does not always point to the right Python environment.
Moreover, your "check" to see if Sionna is installed imports os a second time,
which is not needed I guess (so replacing it with import sys should be fine).

Hence, it is better to refer to sys.executable, which points to the current
Python executable. Additionally, using the ! syntax feels more natural
inside Jupyter Notebooks.

To perform a bulk search-and-replace, I used ripgrep to identify all the files,
and the following Python script to perform the actual edits:

import re
import sys


if __name__ == "__main__":
    search = re.compile(
        r"import os([^o]+)os.system\(\\\"pip install sionna\\\"\)", re.MULTILINE
    )
    replace = r"import sys\1!{sys.executable} -m pip install sionna"

    for file in sys.argv[1:]:
        with open(file, "r", encoding="utf-8") as f:
            content = f.read()
            content = search.sub(replace, content)

        with open(file, "w", encoding="utf-8") as f:
            f.write(content)

Moreover, I manually edited the second pip install sionna
in Sionna_tutorial_part4.ipynb so that it is not ran (because there is already one
at the top of the document).

I hope that you don't mind this PR that comes a bit out of nowhere :-)

Checklist

  • Detailed description
  • [ ] Added references to issues and discussions
  • Added / modified documentation as needed
  • [ ] Added / modified unit tests as needed
  • [ ] Passes all tests
  • [ ] Lint the code
  • Performed a self review
  • Ensure you Signed-off the commits. Required to accept contributions!
  • [ ] Co-authored with someone? Add Co-authored-by: user@domain and ensure they signed off their commits too.

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.

1 participant