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

chore(docs): cleanup pip install in notebooks #471

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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