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 import paths in simulation module #838

Merged

Conversation

srstevenson
Copy link
Contributor

Prior to 93af28, the simulation directory was added to PYTHONPATH by appending to sys.path at the start of each file, before submodules of simulation such as core and interfaces were imported.

In 93af28, the calls to sys.path.append were moved below the import block and so relative imports of core and interfaces no longer resolve and the simulator fails to start with errors such as:

  File "/home/scott/projects/streaming/.venv/lib64/python3.12/site-packages/streamlit/runtime/scriptrunner/exec_code.py", line 88, in exec_func_with_error_handling
    result = func()
             ^^^^^^
  File "/home/scott/projects/streaming/.venv/lib64/python3.12/site-packages/streamlit/runtime/scriptrunner/script_runner.py", line 579, in code_to_exec
    exec(code, module.__dict__)
  File "/home/scott/projects/streaming/simulation/interfaces/sim_ui.py", line 17, in <module>
    from core.create_index import create_stream_index
ModuleNotFoundError: No module named 'core'

This commit prefixes the submodule imports with simulation. so the imports can be resolved, and without needing to manipulate sys.path. This works even when running the simulator outside the source repository, as setuptools.find_packages is used in setup.py to find the modules to package, and this finds the simulation module.

Prior to 93af28, the `simulation` directory was added to `PYTHONPATH` by
appending to `sys.path` at the start of each file, before submodules of
`simulation` such as `core` and `interfaces` were imported.

In 93af28, the calls to `sys.path.append` were moved below the import
block and so relative imports of `core` and `interfaces` no longer
resolve and the simulator fails to start with errors such as:

```
  File "/home/scott/projects/streaming/.venv/lib64/python3.12/site-packages/streamlit/runtime/scriptrunner/exec_code.py", line 88, in exec_func_with_error_handling
    result = func()
             ^^^^^^
  File "/home/scott/projects/streaming/.venv/lib64/python3.12/site-packages/streamlit/runtime/scriptrunner/script_runner.py", line 579, in code_to_exec
    exec(code, module.__dict__)
  File "/home/scott/projects/streaming/simulation/interfaces/sim_ui.py", line 17, in <module>
    from core.create_index import create_stream_index
ModuleNotFoundError: No module named 'core'
```

This commit prefixes the submodule imports with `simulation.` so the
imports can be resolved, and without needing to manipulate `sys.path`.
This works even when running the simulator outside the source
repository, as `setuptools.find_packages` is used in `setup.py` to find
the modules to package, and this finds the `simulation` module.
@ethantang-db
Copy link
Contributor

This looks reasonable to me right now, kicked off the tests to see if it breaks anthing else (linting having issues is a known issue). @snarayan21 to approve though

Copy link
Collaborator

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

thanks for the PR, lgtm as well

@snarayan21 snarayan21 merged commit e1e28f1 into mosaicml:main Dec 9, 2024
7 checks passed
@srstevenson srstevenson deleted the srs/fix-import-paths-in-simulation branch December 9, 2024 22:04
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