-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Evaluator handshake to trainer #5420
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
feat: Evaluator handshake to trainer #5420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also make type info available here (instead of Any) for all available options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed
| SAGEMAKER_REGION env var or default region. | ||
| sagemaker_session (Optional[Any]): SageMaker session object. If not provided, a default | ||
| session will be created automatically. | ||
| model (Union[str, Any]): Model for evaluation. Can be: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the documentation, on additional values allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completed.
|
|
||
| try: | ||
| # Handle BaseTrainer type | ||
| if hasattr(v, '__class__') and v.__class__.__name__ == 'BaseTrainer' or hasattr(v, '_latest_training_job'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the conditioning of v.__class__.__name__ == 'BaseTrainer' will I able to supply SFTTrainer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous jobs were submitted fine, so might have worked.
I moved the code to model_resolution.py to keep it central in this method _resolve_base_model and updated it to isinstance to be sure.
| benchmark: _Benchmark | ||
| dataset: Union[str, Any] # Required field, must come before optional fields | ||
| subtasks: Optional[Union[str, List[str]]] = None | ||
| evaluate_base_model: bool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make evaluate_base_model false for all evaluations.
Issue #, if available:
Description of changes:
arn:aws:sagemaker:us-west-2:729646638167:training-job/pipelines-ayzeo4u9l04n-EvaluateCustomModel-XO0F6U2FMJarn:aws:sagemaker:us-west-2:729646638167:training-job/pipelines-1ufg9wznju1o-EvaluateCustomModel-o2gwjaKdc2arn:aws:sagemaker:us-west-2:729646638167:training-job/CustomInference-wgvcp7tvxhex-cEW1geWEAABy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.