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 custom_ds_n CLI arg #187

Closed
wants to merge 5 commits into from
Closed

add custom_ds_n CLI arg #187

wants to merge 5 commits into from

Conversation

reaganjlee
Copy link
Collaborator

@reaganjlee reaganjlee commented Apr 13, 2023

Allows --custom_ds_n [custom_name] to shorten long naming from multiple datasets

Unmodified, the output path to eliciting with datasets super_glue boolq and christykoh/boolq_pt
elk-reporters/bigscience/bloomz-7b1-mt/christykoh/boolq_pt, super_glue boolq/quizzical-mccarthy

To cd to this reporter requires, elk-reporters/bigscience/bloomz-7b1-mt/christykoh/"boolq_pt, super_glue boolq"/quizzical-mccarthy where path is divided with arbitrary number of / in the datasets as opposed to where datasets are actually divided

You can use this keyword when running elk elicit/eval without typing the full path to group the experiments by datasets, such as in the example above, --custom_ds_n multiboolq to get elk-reporters/bigscience/bloomz-7b1-mt/multiboolq/quizzical-mccarthy

@@ -35,6 +35,7 @@ class Run(ABC):
cfg: Union["Elicit", "Eval"]
out_dir: Optional[Path] = None
dataset: DatasetDict = field(init=False)
custom_ds_n: Optional[Path] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

random unimportant thing: because we now require Python >= 3.10, we can replace Union[X, Y] with X | Y and Optional[X] with X | None in type annotations

Comment on lines +48 to 50
if self.custom_ds_n:
ds_name = self.custom_ds_n
root = elk_reporter_dir() / self.cfg.data.model / ds_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we would need to do the same for models too.
However, I feel like, what we want instead maybe, is to be able to set a custom path for the whole root ... or at least what follows after elk_reporter_dirt()

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@norabelrose
Copy link
Member

hmm it's not really clear to me that we need this in addition to the custom out_dir

@reaganjlee reaganjlee deleted the renaming branch August 18, 2023 07:54
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.

None yet

4 participants