SalesforceAIResearch / uni2ts

Unified Training of Universal Time Series Forecasting Transformers
Apache License 2.0
868 stars 94 forks source link

Change to python3 for black in pre-commit #73

Closed liu-jc closed 4 months ago

liu-jc commented 5 months ago

The previous language version in https://github.com/SalesforceAIResearch/uni2ts/blob/main/.pre-commit-config.yaml is python3.10. When we use python3.11, it would throw an error. So, I relaxed to python3 as suggested in https://github.com/pre-commit/pre-commit/issues/1761#issuecomment-763028818.

chenghaoliu89 commented 5 months ago

@liu-jc, this is a good goal, but I dont think it can be achieved with just simple modifications of python 3.10 -> python 3. Because the current dependent libraries and the versions maybe not compatible for different python version.

First, we should have more unit test for specific features. Second, @liu-jc, if you want to relax the constraints for both python 3.10, 3.11, can you test the compatibility for the current dependencies? Third, @gorold can we add specific python version for the testing of github action workflow.

liu-jc commented 5 months ago

@liu-jc, this is a good goal, but I dont think it can be achieved with just simple modifications of python 3.10 -> python 3. Because the current dependent libraries and the versions maybe not compatible for different python version.

First, we should have more unit test for specific features. Second, @liu-jc, if you want to relax the constraints for both python 3.10, 3.11, can you test the compatibility for the current dependencies? Third, @gorold can we add specific python version for the testing of github action workflow.

@chenghaoliu89, this is only for black in pre-commit. I didn't want to relax the python version requirement to python 3. In our dependencies requirements, we require python>=3.10: https://github.com/SalesforceAIResearch/uni2ts/blob/adf72061666813456200f3bf083c8389eaca4bfe/pyproject.toml#L28 But the user can have python 3.11 or python 3.12, which is happened on my GCP. Then in this case, if you commit the code, black will throw error saying cannot find the interpreter for python 3.10.

I don't think this is something related to unit testing. For unit test, I think we have specified the version as python 3.10. https://github.com/SalesforceAIResearch/uni2ts/blob/adf72061666813456200f3bf083c8389eaca4bfe/.github/workflows/test.yml#L13C11-L13C33

liu-jc commented 5 months ago

I don't foresee any potential problems with doing this change. It's also suggested by the author of pre-commit in a similar issue https://github.com/pre-commit/pre-commit/issues/1761#issuecomment-763028818.

chenghaoliu89 commented 5 months ago

@liu-jc, this is a good goal, but I dont think it can be achieved with just simple modifications of python 3.10 -> python 3. Because the current dependent libraries and the versions maybe not compatible for different python version. First, we should have more unit test for specific features. Second, @liu-jc, if you want to relax the constraints for both python 3.10, 3.11, can you test the compatibility for the current dependencies? Third, @gorold can we add specific python version for the testing of github action workflow.

@chenghaoliu89, this is only for black in pre-commit. I didn't want to relax the python version requirement to python 3. In our dependencies requirements, we require python>=3.10:

https://github.com/SalesforceAIResearch/uni2ts/blob/adf72061666813456200f3bf083c8389eaca4bfe/pyproject.toml#L28

But the user can have python 3.11 or python 3.12, which is happened on my GCP. Then in this case, if you commit the code, black will throw error saying cannot find the interpreter for python 3.10. I don't think this is something related to unit testing. For unit test, I think we have specified the version as python 3.10. https://github.com/SalesforceAIResearch/uni2ts/blob/adf72061666813456200f3bf083c8389eaca4bfe/.github/workflows/test.yml#L13C11-L13C33

Thanks @liu-jc for clarification of the black in pre-commit.