CrystaLLM
CrystaLLM copied to clipboard
Fix Module Import Issue in preprocess.py for Enhanced Compatibility
Changes Made
- Modified
sys.path
withinpreprocess.py
to dynamically include the path to thecrystallm
module. This change resolves theModuleNotFoundError
by ensuring Python can locate and importcrystallm
regardless of the current working directory.
Rationale
The preprocess.py
script previously assumed a specific working directory for successful module importation. This assumption could lead to execution errors when the script was run from a different directory, limiting its flexibility and usability. By dynamically adjusting the sys.path
, we cater to a broader range of use cases, making the script more robust and user-friendly.
I welcome feedback on this change, particularly regarding:
- Any potential impacts on existing workflows I might have overlooked.
- Suggestions for further testing or scenarios to consider ensuring the robustness of this fix.
Thank you for considering this contribution.
Thank you for making this contribution. I agree, this approach is more flexible, and allows the script to be run from any directory. I don't see any issues with this change. However, there are a number of other scripts in the bin
folder which assume a working directory; for example: deduplicate.py
, generate_cifs.py
. I think it would be good to change those scripts as well to use the proposed approach.
Since there are a number of scripts that need to be updated, perhaps we can create a path_setup.py
file in bin
with the contents:
import os
import sys
def setup_path():
script_dir = os.path.dirname(os.path.abspath(__file__))
parent_dir = os.path.abspath(os.path.join(script_dir, '..'))
if parent_dir not in sys.path:
sys.path.insert(0, parent_dir)
and then call setup_path()
from the relevant scripts:
from path_setup import setup_path
setup_path()
I raised an issue related to ModuleNotFoundError
. I have a question. Why don't we install crystalllm
as a module in a conda environment? In the current method, you need to add append this dynamic path in other scripts in bin
.
I agree with @kianpu34593 that installing crystallm
as a module is a way to solve this problem. I have created PR #6 which adds a setup.py
file and an extra installation step. @hoon-ock Please let me know if this alternate approach addresses your issue.