Conversation
shchur
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.