ML-Agents Upgrade to 1.1.0/Release 22 (ml-agents-envs)

[benaslater]

PR Description

Upgrade ML-Agents to 1.1.0 from 0.30.0.experimental (ml-agents-envs), latest release 22.

Proposed change(s)

Update ML-Agents Unity-backend to 3.0.0 (done), update Python to min 3.10.0 and update PyPI packages accordingly with this PR.

This PR simply does the following:

1. Bump ml-agents-envs to 1.0.0 minimum (006cee6)
2. Updated toml.py to remove accepted python versions < 3.10 (06f7423),
3. Bump minimum Python 3.10.0 and exclusive to 3.10.13, as per ml-agents package requirement (2dddd4c),
4. Minor changes to Readme.md and project structure (f543d1f, 37baa9c),
5. Add BadGoalMulti to list of accepted raycast objects (dc96a54).
6. Bumped PyPI project version to 5.0.0 due to breaking changes (4d54395).

There are minor changes that add extra functionality such as the official no-graphics output, which was unofficially supported by ml-agents previously. The major takeaway is that we are transitioning to python 3.10 for future releases of ml-agents and can utilise no graphics output for training sessions. Unity editor minimum was bumped to 2023 and numpy has also been bumped, helping to reduce dependency conflicts.

Useful links (Github issues, discussion threads, etc.)

Closes #5.

Types of change(s)

• Bug fix
• New feature
• Code refactor
• Breaking change
• Documentation update
• Other (ML-Agents Upgrade)

Checklist

• Added tests that prove my fix is effective or that my feature works
• Updated the changelog (if applicable)
• Updated the documentation (if applicable)
• Updated the migration guide (if applicable)

Other comments (if any)

Screenshots (if any)

[alhasacademy96]

This PR needs to be more informative. What are the changes and how will they affect our implementation of mlagents for AAI? (i.e. new functionality, bug fixes, changes, potential conflicts, etc):
https://github.com/Unity-Technologies/ml-agents/releases/tag/release_22.

Edit the PR description and explain the changes in detail (there are important changes/additions such as "Added no_graphics support", "Bumped numpy version to >=1.23.5,<1.24.0", "The minimum supported Unity version was updated to 2023.2.", etc.) These changes really need to be detailed out and explained how they would change the functionality of AAI python side of things (and potentially unity side too).

We're only using the ml-agents-env sub package for your reference.

[benaslater]

This PR needs to be more informative. What are the changes and how will they affect our implementation of mlagents for AAI? (i.e. new functionality, bug fixes, changes, potential conflicts, etc):

I don't agree; this would mean for every dependency version bump developers would have to duplicate the changelog into the PR description. Where I think it is useful to include details like this in a dependency version bump PR is if the version is being bumped to make use of a particular feature/big fix. I see this PR instead just as a "the version hasn't been updated in ages and so desperately needs doing" PR. I added another sentence to the main PR to illustrate this a bit more.

These changes really need to be detailed out and explained how they would change the functionality of AAI python side of things (and potentially unity side too).

Hopefully not at all*, except we will need a major version bump for this change since it's breaking (because of the python version bump).

I am a bit suspicious that the TOML isn't going to play nice - I think the dockerising the E2E tests stuff you're working on will be really useful for unearthing things like that.

• the only customer facing change I can see is that annoyingly the camera outputs are transposed in the new version. This would affect users who are e.g. trying to run a pretrained agent across the major version bump this change needs - but they shouldn't be doing that anyway

[alhasacademy96]

As this PR is mainly focused on upgrading the mlagents version (the version we are using works fine, we need to evaluate and justify the benefits of upgrading to the latest version). (For example) You mentioned the camera outputs are transposed in 3.0.0, what's the implication here, how will this affect AAI? People who use AAI and are not technical need to know (myself and the other members will know what this means but it's for other current/future members to see what the change implies). Numpy version bump is also a massive one as we've had major difficulties with dependency-related conflicts in the past, mainly to do with numpy being incompatible with other packages such as PyTorch.

You need to detail these out here along with the other changes that will come along as we need to understand how all of this will affect the use of mlagents for AAI. Please also use the PR template in the repo, it's there for a reason. This PR description is unacceptable for a major (breaking) change for this repo. It's not a simple change you're working on (i.e. a patch) which would be OK with this description but this isn't the case here.

PR Description

This is the python part of the mlagents upgrade. It is one of three PRs that should be reviewed and merged together. See details at Kinds-of-Intelligence-CFI/animal-ai-unity#44

This PR needs to be more informative. What are the changes and how will they affect our implementation of mlagents for AAI? (i.e. new functionality, bug fixes, changes, potential conflicts, etc):

I don't agree; this would mean for every dependency version bump developers would have to duplicate the changelog into the PR description. Where I think it is useful to include details like this in a dependency version bump PR is if the version is being bumped to make use of a particular feature/big fix. I see this PR instead just as a "the version hasn't been updated in ages and so desperately needs doing" PR. I added another sentence to the main PR to illustrate this a bit more.

I strongly disagree with your statement:

I see this PR instead just as a "the version hasn't been updated in ages and so desperately needs doing" PR.
Mlagents was not updated in ages as there was nothing to update to for a while (mlagents is deprecated). The latest version came online last October and the newest a few weeks ago. We shouldnt just upgrade mlagents just for the sake of it. We need to have reasons as the version were using works and is not broken. See my issue: #5. I've specifically mentioned to evaluate mlagents first before upgrading. Note that this issue is for mlagents 3.0.0.exp.1.

Again, this PR should be a standalone and have its own appropriate and detailed PR. Currently, it's not and it can't be merged.

[benaslater]

the version we are using works fine, we need to evaluate and justify the benefits of upgrading to the latest version

In general we should try to keep dependencies as up to date as possible. This version was trapping on Python 3.9, so I don't agree that it works fine.

You mentioned the camera outputs are transposed in 3.0.0, what's the implication here, how will this affect AAI?

I can't think of any significant impacts from the transposition - are there any on your mind that you're worried about?

we've had major difficulties with dependency-related conflicts in the past, mainly to do with numpy being incompatible with other packages such as PyTorch

Being stuck on Python 3.9 probably didn't help, so hopefully this bump will help smooth things out a bit dependency-wise. Since I'm doing a fresh build to bump the unity editor version again, I'll make a fresh pyenv to double check for any version conflict issues.

Please also use the PR template in the repo, it's there for a reason. This PR description is unacceptable for a major (breaking) change for this repo.

I'm sorry that you find the PR description I wrote unacceptable. Given that the change had to be made across three repos, I think it's reasonable to have one PR carry the main detail of the change and the others to reference it (other developers I've worked with didn't seem to take issue with this). Perhaps you could take a look at the description of the unity PR where I included the detail, and draft a suggestion of what you would have liked to see here, so that I have a better understanding of what you're looking for?

We shouldnt just upgrade mlagents just for the sake of it. We need to have reasons as the version were using works and is not broken.

Users were having issues with being pinned to an old version of Python. These kinds of issues tend to accumulate when dependencies aren't updated for a while.

Again, this PR should be a standalone and have its own appropriate and detailed PR.

I don't think it can be standalone, because upgrading mlagents on the python side will break the integration with mlagents on the unity side. Do you know a way we could make these changes independently?

[alhasacademy96]

the version we are using works fine, we need to evaluate and justify the benefits of upgrading to the latest version

In general we should try to keep dependencies as up to date as possible. This version was trapping on Python 3.9, so I don't agree that it works fine.

Python 3.9 is still used across projects. But, of course it was a limitation so I agree on that.

You mentioned the camera outputs are transposed in 3.0.0, what's the implication here, how will this affect AAI?

I can't think of any significant impacts from the transposition - are there any on your mind that you're worried about?

None that come to mind but it's still a topic to discuss. For example, how would this affect training instances? Or do we simple need to update the documentation to reflect this new change so users don't get surprised?

we've had major difficulties with dependency-related conflicts in the past, mainly to do with numpy being incompatible with other packages such as PyTorch

Being stuck on Python 3.9 probably didn't help, so hopefully this bump will help smooth things out a bit dependency-wise. Since I'm doing a fresh build to bump the unity editor version again, I'll make a fresh pyenv to double check for any version conflict issues.

The conflicts were not related to 3.9 per se, but the way mlagents configured specific dependency versions. But, the python bump means we need to use at least 3.10, which helps with maintenance. Do we need to update the tests in this repo to use 3.10? Have you run these tests?

Please also use the PR template in the repo, it's there for a reason. This PR description is unacceptable for a major (breaking) change for this repo.

I'm sorry that you find the PR description I wrote unacceptable. Given that the change had to be made across three repos, I think it's reasonable to have one PR carry the main detail of the change and the others to reference it (other developers I've worked with didn't seem to take issue with this). Perhaps you could take a look at the description of the unity PR where I included the detail, and draft a suggestion of what you would have liked to see here, so that I have a better understanding of what you're looking for?

Again, this PR should be a standalone and have its own appropriate and detailed PR.

I don't think it can be standalone, because upgrading mlagents on the python side will break the integration with mlagents on the unity side. Do you know a way we could make these changes independently?

No harm done. My main point here is that this change is breaking, and would require a standalone PR (by standalone, I mean that it should have it's own weight), and not just be treated as a simple PR, with a simple description. We need to understand what is being changed here. I've stated what is missing in the PR. Kindly provide these details.

We shouldnt just upgrade mlagents just for the sake of it. We need to have reasons as the version were using works and is not broken.

Users were having issues with being pinned to an old version of Python. These kinds of issues tend to accumulate when dependencies aren't updated for a while.

I was unaware that users were having trouble with 3.9. Anyone in the team and/or external users? What were the issues?