CrystaLLM icon indicating copy to clipboard operation
CrystaLLM copied to clipboard

Fix Module Import Issue in preprocess.py for Enhanced Compatibility

Open hoon-ock opened this issue 11 months ago • 3 comments

Changes Made

  • Modified sys.path within preprocess.py to dynamically include the path to the crystallm module. This change resolves the ModuleNotFoundError by ensuring Python can locate and import crystallm 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.

hoon-ock avatar Mar 26 '24 19:03 hoon-ock

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()

lantunes avatar Mar 27 '24 01:03 lantunes

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.

kianpu34593 avatar Apr 05 '24 13:04 kianpu34593

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.

lantunes avatar Apr 06 '24 13:04 lantunes