castorini / rank_llm

Repository for prompt-decoding using LLMs (GPT3.5, GPT4, Vicuna, and Zephyr)
http://rankllm.ai
Apache License 2.0
277 stars 36 forks source link

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

Open sahel-sh opened 6 months ago

sahel-sh commented 6 months ago

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)
wu-ming233 commented 5 months ago

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

sahel-sh commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

"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 commented 5 months ago

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!