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

Add init syntax for scripts directory #538

Closed
wants to merge 2 commits into from
Closed

Conversation

j316chuck
Copy link
Contributor

@j316chuck j316chuck commented Aug 18, 2023

Add init syntax for scripts directory.

@bmosaicml was running into issues importing the scripts.data_prep.convert_dataset_hf file in the test_training.py test but import scripts was working. This PR should fix these import errors.

_______________________________ ERROR collecting tests/test_training.py ________________________________
ImportError while importing test module '/Users/jeremy.dohmann/LLMWork/llm-foundry/tests/test_training.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_training.py:17: in <module>
    from scripts.data_prep.convert_dataset_hf import main as main_hf  # noqa: E402
E   ModuleNotFoundError: No module named 'scripts.data_prep'

@dakinggg
Copy link
Collaborator

I'd prefer we not do this. Anything that needs to be imported should probably be part of the llmfoundry package properly, and for testing scripts specifically we can just append to the path in the test file so the script main can be imported.

@j316chuck
Copy link
Contributor Author

Agreed closed. Strange that Jeremy saw this bug earlier.

@j316chuck j316chuck closed this Aug 25, 2023
@dakinggg
Copy link
Collaborator

I resolved this with Jeremey earlier. It was an issue specific to his python setup.

@j316chuck j316chuck deleted the chuck/add_init branch October 5, 2023 21:23
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.

2 participants