-
Notifications
You must be signed in to change notification settings - Fork 527
TimesFM #2135
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
base: main
Are you sure you want to change the base?
TimesFM #2135
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
def outputs(self) -> Dict[str, Dict[int, str]]: | ||
return super().outputs |
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.
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.
@@ -2629,6 +2629,51 @@ class EncoderDecoderOnnxConfig(EncoderDecoderBaseOnnxConfig): | |||
DEFAULT_ONNX_OPSET = 14 # uses SDPA in Transformers, hence opset>=14. | |||
|
|||
|
|||
class TimesFMDummyInputGenerator(DummyInputGenerator): |
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.
could make sense to re-use DummyPatchTSTInputGenerator
instead (with some extra modifications), let's wait for huggingface/transformers#34082 to be merged
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.
@echarlaix one question i had about the huggingface/transformers#34082 in the timesfm_config i do not think my TimesFmOnnxConfig
is correct... do you agree?
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.
Yes it shouldn't inherit from OnnxSeq2SeqConfigWithPast
(which should be used for the export of encoder-decoder models). I think you can directly remove TimesFmOnnxConfig
in huggingface/transformers#34082 as it shouldn't be used anywhere from what I know (also I'm expecting anything onnx related to be deprecated in v5)
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.
in any case let me know if I can help @kashif
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.
ok let me remove it... it would be good to then get it working via transformers.js... let me remove it first and then get back to you!
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.
sure thanks, also cc @michaelbenayoun @JingyaHuang @IlyasMoutawwakil who can take a look while I'm ooo
thanks! |
|
||
@property | ||
def inputs(self) -> Dict[str, Dict[int, str]]: | ||
return {"past_values": {0: "batch_size", 1: "sequence_length"}} |
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.
an issue is remaining before we can merge : it seems that the resulting onnx graph doesn't considered the batch_size
as dynamic (won't accept one different than the one given during export) likely coming from :
@kashif any chance we can add the modification directly in the transformers modeling ? otherwise we need to patch the model before export
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.
checking but i believe so
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.
Indeed, that does look to be the source of the loss of dynamic shapes. torch.onnx.export doesn't play well with python list operations, so we'd need to refactor it to use tensor operations, if possible.
What does this PR do?
Add support for TimesFM model