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

P3-Find a better way for including rank_llm in sys path. #53

Open
sahel-sh opened this issue Jan 25, 2024 · 6 comments
Open

P3-Find a better way for including rank_llm in sys path. #53

sahel-sh opened this issue Jan 25, 2024 · 6 comments
Assignees

Comments

@sahel-sh
Copy link
Member

Currently we add the following snippet to every script that we want to run both from a cloned repo and using package installation.
Ideally we should find a cleaner/simpler way

import sys
import os

SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
parent = os.path.dirname(SCRIPT_DIR)
parent = os.path.dirname(parent)
sys.path.append(parent)
@sahel-sh sahel-sh changed the title Find a better way for including rank_llm in sys path. P3-Find a better way for including rank_llm in sys path. Jan 29, 2024
@wu-ming233
Copy link
Contributor

I can handle this with relative paths. Would that be preferred over the current snippet?

@sahel-sh
Copy link
Member Author

Relative or absolute path would be still the same, @wu-ming233 if you are interested in this problem. Try looking for solutions that don't require adding path at the beginning of every file that needs it.

@wu-ming233
Copy link
Contributor

By relative imports, I meant imports such as:

from ..retrieve.indices_dict import INDICES
from ..retrieve.topics_dict import TOPICS

as opposed to, say,

SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
parent = os.path.dirname(SCRIPT_DIR)
parent = os.path.dirname(parent)
sys.path.append(parent)

from rank_llm.result import Result, ResultsWriter
from rank_llm.retrieve.indices_dict import INDICES
from rank_llm.retrieve.topics_dict import TOPICS

Do you think that will be preferred?

@ronakice
Copy link
Member

I feel relative imports are fine internally, this is what transformers does from what I remember.

What about generally relying more on init.py for conveniently importing modules besides this? @sahel-sh

@sahel-sh
Copy link
Member Author

"Remember that you should generally opt for absolute imports over relative ones, unless the path is complex and would make the statement too long" this is from real python: https://realpython.com/absolute-vs-relative-python-imports/#:~:text=With%20your%20new%20skills%2C%20you,make%20the%20statement%20too%20long.

@wu-ming233 In general I would like to keep the imports as absolute path. I will try a relative path in one of the demos. If the demo is still runnable both from a cloned repo and by importing rankllm after installing it as a package, I will let you know and you can update every thing script that does add to the path explicitly?

@wu-ming233
Copy link
Contributor

If we were to use relative imports, one can call our scripts as modules, respecting the package structure, such as:

python -m rank_llm.demo.rerank_demo_docs.py

which would work if the user calls from the directory rank_llm/src.

I am still looking for ways that could handle imports more elegantly, but most of them don't seem to be as robust as just using the somewhat dirty sys.path.append. The most "pythonic" way so far is to use editable installs, though this would make it slightly feel like "development mode" even when the user just wants to run a demo. Let me know what you think!

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

No branches or pull requests

3 participants