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

Bugfix/fix old tf architectures #552

Merged
merged 13 commits into from
Jun 15, 2022
Merged

Conversation

KaleabTessera
Copy link
Contributor

What?

  • Fix state based, networked and centralised architectures.
  • Remove lp as independent dependency. It is a core dependency in acme, so for now, it is auto-downloaded with acme.
  • Updated some core libraries.
  • Change env_states to s_t for consistency.
  • There is still a weird bug that occurs when running tests with shared_weights=False with multiple other tests. Full description of bug:
    Running tests with shared_weights=False pass when running indepedently
                (other tests commented out), but fail when run with other tests and not
                enough parallel cores (fail with 2 or less cores, pass with 4 cpu cores). This is likely a race condition,
                hangling process from previous tests or related to network sampling
                (TODO @Dries investigate if you have a chance). Only the test fails,
                the examples run.
    
    For now, I don't think it worth fixing these kind of tests since tf will be deprecated soon.

Why?

  • So that tests pass and we can do final tf release.

How?

Extra

mmorris44
mmorris44 previously approved these changes Jun 9, 2022
Copy link
Contributor

@mmorris44 mmorris44 left a comment

Choose a reason for hiding this comment

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

Looks good @KaleabTessera. Merge pending benchmarks.

Also, I had a few minor comments which you may choose to address if you'd like.

Dockerfile Show resolved Hide resolved
README.md Show resolved Hide resolved
mava/components/tf/architectures/networked.py Show resolved Hide resolved
tests/systems/mad4pg_system_test.py Show resolved Hide resolved
tests/systems/mad4pg_system_test.py Show resolved Hide resolved
tests/systems/maddpg_system_test.py Show resolved Hide resolved
Copy link
Collaborator

@RuanJohn RuanJohn left a comment

Choose a reason for hiding this comment

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

Thank you @KaleabTessera! Benchmarks show no system performance degrading.

@KaleabTessera KaleabTessera merged commit d2a4004 into develop Jun 15, 2022
@KaleabTessera KaleabTessera deleted the bugfix/fix-old-tf-architectures branch June 15, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants