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

Input, Output and Adapter refactor #1280

Merged
merged 20 commits into from
Jan 6, 2023
Merged

Input, Output and Adapter refactor #1280

merged 20 commits into from
Jan 6, 2023

Conversation

teetone
Copy link
Member

@teetone teetone commented Dec 31, 2022

Changes

  • input of Instance is now an object.
  • output of Reference is now an object.
  • Broke apart Adapter by creating a class for each adaptation method. Adapter is now an abstract class with GenerationAdapter, LanguageModelingAdapter, MultipleChoiceJointAdapter, MultipleChoiceSeparateAdapter, MultipleChoiceCalibratedAdapter, and BinaryRankingAdapter child classes.
  • Moved out AdapterSpec, Prompt, RequestState, and ScenarioState from adapter.py to separate files.
  • Added text_code to ModelRunExpander for reasoning scenarios. We can't assume all models are text-only.
  • Some minor cleanup here and there.

The changes in this PR should not invalidate our cache. I did a full run with the offline models to ensure the changes in this PR are safe.

@percyliang
Copy link
Contributor

Wow, this is great! Since this is such a big PR, would you mind going through and putting a few comments to help understand what the changes are (e.g., this piece of code is new or simply moved from another file)?

@teetone
Copy link
Member Author

teetone commented Jan 2, 2023

Wow, this is great! Since this is such a big PR, would you mind going through and putting a few comments to help understand what the changes are (e.g., this piece of code is new or simply moved from another file)?

I did a big run with the changes in this PR for the offline eval models, and the cache seems to be safe. I will work on adding comments on this PR next.

Copy link
Collaborator

@rishibommasani rishibommasani left a comment

Choose a reason for hiding this comment

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

LGTM fwiw - did you test, since I noticed there are some minor changes to the scenario files beyond the main Input/Output change (e.g. renaming variables in wikfact), so curious just to make sure it works.

src/helm/benchmark/scenarios/newsqa_scenario.py Outdated Show resolved Hide resolved
src/helm/benchmark/run_expander.py Outdated Show resolved Hide resolved
Copy link
Member Author

@teetone teetone left a comment

Choose a reason for hiding this comment

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

@percyliang I added comments in most files

@@ -0,0 +1,86 @@
from dataclasses import dataclass, field
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved out Substitution and AdapterSpec out from the old adapter.py to their own file.

@@ -0,0 +1,65 @@
import re
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved out Prompt out from the old adapter.py to its own file.

@@ -0,0 +1,75 @@
from dataclasses import dataclass
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved out RequestState out of the old adapter.py to its own file.

@@ -0,0 +1,40 @@
from collections import defaultdict, OrderedDict
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved out ScenarioState out from the old adapter.py to its own file.

@@ -66,18 +66,18 @@ def apply(self, instance: Instance, seed: Optional[int] = None) -> Instance:
"""
rng: Random = self.get_rng(instance, seed)

perturbed_instance: str = instance.input
Copy link
Member Author

Choose a reason for hiding this comment

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

input of Instance is now an object instead of a string. Also, the variable nameperturbed_instance was a bit confusing.

from helm.common.request import Request
from .in_context_learning_adapter import InContextLearningAdapter


Copy link
Member Author

Choose a reason for hiding this comment

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

The logic in this file was from the old Adapter.

from helm.benchmark.adaptation.adapter_spec import AdapterSpec
from .adapter_factory import AdapterFactory, ADAPT_GENERATION


Copy link
Member Author

Choose a reason for hiding this comment

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

Was part of the old test_adapter.py file.

from helm.benchmark.adaptation.adapter_spec import AdapterSpec
from .adapter_factory import AdapterFactory, ADAPT_LANGUAGE_MODELING


Copy link
Member Author

Choose a reason for hiding this comment

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

Was part of the old test_adapter.py file.

from helm.benchmark.adaptation.adapter_spec import AdapterSpec
from .adapter_factory import AdapterFactory, ADAPT_MULTIPLE_CHOICE_JOINT


Copy link
Member Author

Choose a reason for hiding this comment

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

Was part of the old test_adapter.py file.

from helm.benchmark.adaptation.adapter_spec import AdapterSpec
from helm.benchmark.window_services.tokenizer_service import TokenizerService
from .adapter import Adapter
from .generation_adapter import GenerationAdapter
Copy link
Member Author

Choose a reason for hiding this comment

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

All Adapters live in src/helm/benchmark/adaptation/adapters.

@teetone
Copy link
Member Author

teetone commented Jan 2, 2023

LGTM fwiw - did you test, since I noticed there are some minor changes to the scenario files beyond the main Input/Output change (e.g. renaming variables in wikfact), so curious just to make sure it works.

Yes, I tested extensively. I did a full run with the Together and TNLGv2 models to ensure the cache is safe.

Copy link
Contributor

@percyliang percyliang left a comment

Choose a reason for hiding this comment

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

Thanks again for the big refactor! Everything looks good - only thing is making TextInput separate from Input.

@teetone
Copy link
Member Author

teetone commented Jan 5, 2023

I will do another big rerun before I merge this PR.

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