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

Release 5.0 and installation with pip #1153

Merged
merged 36 commits into from
Dec 20, 2023

Conversation

DocGarbanzo
Copy link
Contributor

@DocGarbanzo DocGarbanzo commented Dec 12, 2023

Release 5.0 and installation with pip

This PR is in preparation for the Donkey Car 5.0 release. Compared to current HEAD, this release has the following major changes:

  • a more modern setup.cfg and pyproject.toml installation
  • a migration from a full conda install towards a conda/pip install where we will be publishing a wheel file to pypi, such that users can install Donkey Car with pip install donkeycar[XX], where XX contains the platform.
  • removal of the keras-viz dependency from which we only required two utility functions which have now been copied into the code directly
  • improve consistency in tub, such that adding tub with different input or metadata will not be allowed

As part of the 5.X release we will also be updating the documentation accordingly. The changes for the documentation are in https://github.com/autorope/donkeydocs/tree/install_with_pip.

…d rename old setup.py to not infer with new install. This file will be delete ultimately.
…exes which was missing its implementation in the datastore. Added tf to pi install.
… works from withing the donkeycar project directory.
…pandas / matplotlib don't work. Updated version and pushed that to pypi test.
@DocGarbanzo DocGarbanzo marked this pull request as draft December 12, 2023 22:20
@cfox570
Copy link
Contributor

cfox570 commented Dec 13, 2023

I have reviewed the changes included for Release 5.0 and would like to encourage you to include changes to the MacOS setup to leverage the Mac GPU. METAL is the GPU technology that powers hardware-accelerated graphics and AI on Apple platforms. METAL is supported by Mac computers from the year 2012. See METAL on Mac Computers. Apple has released tensorflow-macos and tensorflow-metal to speedup tensorflow on the Mac. I have used this installation on both an INTEL and ARM mac for the last couple of years. Here is a set of instructions I have used for installation on the Macbook Air M1. Install Tensorflow-macos. There is one difference for the ARM Mac. For mac computers prior to 2012, the setup would use standard tensorflow. Once you merge the files into the MAIN branch I will work through suggested changes to the code and instructions for each type of Mac. Maybe you could solicit others with Macs to test.

@DocGarbanzo
Copy link
Contributor Author

I have reviewed the changes included for Release 5.0 and would like to encourage you to include changes to the MacOS setup to leverage the Mac GPU. METAL is the GPU technology that powers hardware-accelerated graphics and AI on Apple platforms. METAL is supported by Mac computers from the year 2012. See METAL on Mac Computers. Apple has released tensorflow-macos and tensorflow-metal to speedup tensorflow on the Mac. I have used this installation on both an INTEL and ARM mac for the last couple of years. Here is a set of instructions I have used for installation on the Macbook Air M1. Install Tensorflow-macos. There is one difference for the ARM Mac. For mac computers prior to 2012, the setup would use standard tensorflow. Once you merge the files into the MAIN branch I will work through suggested changes to the code and instructions for each type of Mac. Maybe you could solicit others with Macs to test.

@cfox570 - this is a very good comment. Having a quick look at your gist above, it looks like we should simply replace tensorflow-macos==2.9 with tensorflow-metal==0.5 in the setup.cfg for the macos target (matching version from here: https://pypi.org/project/tensorflow-metal/). Let's get this merged and then we can do a 5.1 release with the tensorflow metal upgrade thereafter.

@cfox570
Copy link
Contributor

cfox570 commented Dec 13, 2023

this is a very good comment. Having a quick look at your gist above, it looks like we should simply replace tensorflow-macos==2.9 with tensorflow-metal==0.5 in the setup.cfg for the macos target (matching version from here: https://pypi.org/project/tensorflow-metal/). Let's get this merged and then we can do a 5.1 release with the tensorflow metal upgrade thereafter.

For Conda, the ARM Mac (M1/M2/M3) needs tensorflow-deps installed via conda not pip. Not sure how you change your setup to accomplish.

@DocGarbanzo
Copy link
Contributor Author

this is a very good comment. Having a quick look at your gist above, it looks like we should simply replace tensorflow-macos==2.9 with tensorflow-metal==0.5 in the setup.cfg for the macos target (matching version from here: https://pypi.org/project/tensorflow-metal/). Let's get this merged and then we can do a 5.1 release with the tensorflow metal upgrade thereafter.

For Conda, the ARM Mac (M1/M2/M3) needs tensorflow-deps installed via conda not pip. Not sure how you change your setup to accomplish.

This is already part of this PR. Everything now gets installed via pip. Going from tensorflow-macos to tensorflow-metal is a one line change in setup.cfg.

@DocGarbanzo DocGarbanzo marked this pull request as ready for review December 13, 2023 21:02
@DocGarbanzo DocGarbanzo requested a review from Ezward December 13, 2023 21:02
@cfox570
Copy link
Contributor

cfox570 commented Dec 13, 2023 via email

@cfox570
Copy link
Contributor

cfox570 commented Dec 18, 2023

I tried out the installation of [torch]. I found two issues:

  1. replace PyTorch with just torch
  2. pytorch-lightning is missing

Also I suggest you update the mac instructions to install [macos] instead of [pc]

@DocGarbanzo
Copy link
Contributor Author

I tried out the installation of [torch]. I found two issues:

1. replace PyTorch with just torch

2. pytorch-lightning is missing

Also I suggest you update the mac instructions to install [macos] instead of [pc]

Thanks @cfox570 - we have tested on Mac Intel with the standard TF install that is in the [pc] configuration and everything works fine with that. I'll give it a try to with the [macos] target and see if this works and makes a difference, but this is independent of the code release here.

@@ -78,8 +78,7 @@ def write_record(self, record=None):
# Private properties
contents['_timestamp_ms'] = int(round(time.time() * 1000))
contents['_index'] = self.manifest.current_index
contents['_session_id'] = self.manifest.session_id

contents['_session_id'] = self.manifest.session_id[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the index of 1 here? It's magic-number-ish. Maybe worth a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, saw that comment too late... session_id now returns a tuple of the (number, session_id_string) which allows to simplify the code a lot, so we are picking the string here.

Copy link
Contributor

@Ezward Ezward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good. Thanks so much for this. I'm sorry I took so long to review. I added a couple of question comments, but nothing that blocks release.

@DocGarbanzo DocGarbanzo merged commit 0182bb8 into autorope:main Dec 20, 2023
3 checks passed
@DocGarbanzo DocGarbanzo deleted the install_with_pip branch January 19, 2024 21:46
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.

3 participants