Skip to content

Fix Module Import Issue in preprocess.py for Enhanced Compatibility#4

Open
hoon-ock wants to merge 5 commits into
lantunes:mainfrom
hoon-ock:fix-crystallm-import-issue
Open

Fix Module Import Issue in preprocess.py for Enhanced Compatibility#4
hoon-ock wants to merge 5 commits into
lantunes:mainfrom
hoon-ock:fix-crystallm-import-issue

Conversation

@hoon-ock

Copy link
Copy Markdown

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.

@lantunes

Copy link
Copy Markdown
Owner

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

Comment thread bin/preprocess.py
import os
import sys
sys.path.append(".")
# sys.path.append(".")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This commented line can be deleted.

@kianpu34593

Copy link
Copy Markdown

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.

@lantunes

lantunes commented Apr 6, 2024

Copy link
Copy Markdown
Owner

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.

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