-
Notifications
You must be signed in to change notification settings - Fork 13
Add tirex #17
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 tirex #17
Conversation
shchur
left a comment
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.
Just a few minor comments, otherwise LGTM. The API of the ForecastModel class looks pretty clean, so the wrapper is quite easy to understand.
| - pip: | ||
| - --index-url https://download.pytorch.org/whl/cu126 | ||
| - --extra-index-url https://pypi.org/simple | ||
| - torch~=2.6.0 |
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.
It looks like the requirements.txt and requirements_py26.yaml define slightly different version ranges. Are both of these files supposed to be installed into the same environment, or do they provide two alternative options for installing the dependencies?
I would recommend to define
- one
requirements.txtfile pinning the major dependencies (e.g.,tirexpackage version, pytorch) - the currentrequirements.txtfile is already perfect - one file with an exact snapshot of the dependencies used to run the benchmark (e.g., conda environment with exact
==package versions used during environment, or auvlock file https://docs.astral.sh/uv/pip/compile/#locking-requirements)
The naming requirements_py26.yaml is also a bit strange as it seems to imply that these a requirements for Python 2.6.
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.
So in general it should work just installing the requirements.txt, however, we recommend installing TiRex inside the provided conda environment as we observe some people have problems with CUDA stuff otherwise. (TiRex uses custom CUDA kernels of the sLSTM)
I relaxed the major dependencies lock of the requirements.txt, and added a conda export of the environment on which i ran the test (freezed_test_environment.yaml)
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 my opinion, it's okay to only keep requirements.txt and the frozen_test_environment.yaml. I would recommend pointing to the official installation instructions in the TiRex repo (the corresponding environment.yaml). This way neither you nor we will need to worry if the current requirements.yaml file stored in the fev repo is up-to-date.
|
updated the commits with correct commit email adress |
|
the TiRex repository and the Model on HF are public so the PR should work now : ) |
Note: This PR starts working only after https://github.com/NX-AI/tirex is published
Description of changes:
Add TiRex as a new model.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.