Skip to content
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

Change to python3 for black in pre-commit #73

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

liu-jc
Copy link
Contributor

@liu-jc liu-jc commented Jun 14, 2024

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 pre-commit/pre-commit#1761 (comment).

@liu-jc liu-jc requested a review from gorold June 14, 2024 09:29
@liu-jc liu-jc self-assigned this Jun 14, 2024
@chenghaoliu89
Copy link
Contributor

@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
Copy link
Contributor Author

liu-jc commented Jun 16, 2024

@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:

requires-python = ">=3.10"
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
Copy link
Contributor Author

liu-jc commented Jun 16, 2024

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 pre-commit/pre-commit#1761 (comment).

@chenghaoliu89
Copy link
Contributor

@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:

requires-python = ">=3.10"

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.

@liu-jc liu-jc merged commit 92ddd74 into SalesforceAIResearch:main Jun 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants