The Wayback Machine - https://web.archive.org/web/20201126011041/https://github.com/TheAlgorithms/Python/pull/3926
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

Python 3.9 #3926

Merged
merged 9 commits into from Nov 24, 2020
Merged

Python 3.9 #3926

merged 9 commits into from Nov 24, 2020

Conversation

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 22, 2020

Describe your change:

Migrate to Python 3.9 changing the file extensions of all files that require TensorFlow.

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?
  • Upgrade infrastructure

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@cclauss cclauss force-pushed the cclauss:Python_3.9 branch from e95fcd1 to 60a696c Nov 22, 2020
cclauss added 2 commits Nov 22, 2020
@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 23, 2020

TensorFlow will still take some time as they have a few dependencies as well. Also, mypy is not fully compatible with 3.9. So, I think we should wait for a bit. As for the typing errors, we should use the 3.9 with future import, and then once mypy is fully compatible and all our dependencies have upgraded to 3.9, then we will remove the future import and start running mypy and upgrade the CI to 3.9.

@cclauss
Copy link
Member Author

@cclauss cclauss commented Nov 23, 2020

My strong preference would be that this repo migrates to the current version of Python and leave the following algorithms out of our test cycle because their dependencies do not yet support Python 3.9. I would much rather ensure that our code is tested on and compatible with the current Python than to continue to wait. Six+ weeks is long enough especially given CPython's long alpha, beta, release candidate cycle.

Note: The following files do not have .py extensions because TensorFlow does not yet support the current Python.

  • dynamic_programming/k_means_clustering_tensorflow.py_tf
  • machine_learning/lstm/lstm_prediction.py_tf
  • neural_network/gan.py_tf
  • neural_network/input_data.py_tf
@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 23, 2020

I am a bit hesitant to do monkey patching on an entire repository as that might lead to an unforeseen situation in the future. Also, not everyone is going to have the latest Python installed on their machine let alone use it.

On a side note, any idea as to why the test is taking 18 minutes to download the dependecies?

@cclauss
Copy link
Member Author

@cclauss cclauss commented Nov 23, 2020

monkey patching on an entire repository

I do not think that changing the file extension on four files constitutes monkey patching on an entire repository.

The build times are about the lack of wheels on some dependencies. Does your build cache help us to resolve that issue or will we always have long builds?

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 23, 2020

Try pushing an empty commit and we will check but even without cache it shouldn't take this much time.

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 24, 2020

Closing and reopening the pull request to trigger workflow runs.

@dhruvmanila dhruvmanila reopened this Nov 24, 2020
@cclauss
Copy link
Member Author

@cclauss cclauss commented Nov 24, 2020

Pip process is 3 minutes with caching... I can live with that.

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 24, 2020

There's some problem with the wheels of qiskit-aer: https://github.com/TheAlgorithms/Python/pull/3926/checks?check_run_id=1447346191#step:5:150 otherwise the build would've completed in less than 2 minutes.

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 24, 2020

Let's restrict qiskit to 3.8 as well.
Qiskit is yet to support 3.9: https://github.com/Qiskit/qiskit/blob/master/setup.py#L54
Issue: Qiskit/qiskit#1111

dhruvmanila and others added 2 commits Nov 24, 2020
@cclauss
Copy link
Member Author

@cclauss cclauss commented Nov 24, 2020

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 24, 2020

Well then we don't need to restrict qiskit. We will be fine with 3 minutes then.

@cclauss
Copy link
Member Author

@cclauss cclauss commented Nov 24, 2020

Running on Python 3.9 is different than having a wheel for Python 3.9. The former means that it does not work. The latter means that it works but the install time is long. The tests for all of our quantum algorithms run and pass on Python 3.9. The long install times are dealt with by your pip cache.

@cclauss cclauss merged commit 3fdbf97 into TheAlgorithms:master Nov 24, 2020
2 checks passed
2 checks passed
build
Details
pre-commit
Details
@cclauss cclauss deleted the cclauss:Python_3.9 branch Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.