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

Migrate to Model Registry V2 #144

Merged
merged 35 commits into from
Apr 21, 2023
Merged

Conversation

wduguay-air
Copy link
Contributor

@wduguay-air wduguay-air commented Mar 30, 2023

closes #120

Overview

This pull request migrates the current Model Registry implementation to the Model Registry V2 implementation introduced by the SDK in release v2.5.0.

Testing

The functional tests will be added to the PR after merging the Functional Test PR #143.

  • Functional Test PR merged
  • Functional Tests added

Other changes

  • Modified some existing functional test configs to have a faster execution time.

@wduguay-air wduguay-air self-assigned this Mar 30, 2023
@wduguay-air wduguay-air marked this pull request as ready for review April 12, 2023 20:24
@wduguay-air
Copy link
Contributor Author

wduguay-air commented Apr 12, 2023

Since the last review/conversation, I removed the model_registry implemented locally in cogment-verse, in favor of entirely using the python sdk version. I also change the Model class to implement the same methods (serialize_model, deserialize_model) as other recent cogment projects.

@Yves-air
Copy link

Yves-air commented Apr 12, 2023

Maybe you can have a look at some of the new functionality coming, to see if it would make sense to wait for the next version (excluding enterprise stuff):
https://gitlab.com/ai-r/cogment/cogment-doc/-/merge_requests/155

@wduguay-air
Copy link
Contributor Author

Anyways I was planning to wait a bit to wait for Ha to merge some agent implementations PRs that have been open for a long time. But for now, the current functionalities seem enough for the general cogment-verse use case. I will probably make a new PR in the near future to include them!

@wduguay-air
Copy link
Contributor Author

All implementations PRs I was waiting for are merged. This is ready.

@wduguay-air wduguay-air requested review from saikrishna-1996 and cloderic and removed request for cloderic and saikrishna-1996 April 17, 2023 12:34
@Yves-air
Copy link

Yves-air commented Apr 17, 2023

I would suggest you go through and change model_version for model_iteration everywhere.

The code also does not follow our coding guidelines, but it might be too much for this PR.

@wduguay-air
Copy link
Contributor Author

I just made a pass to rename to model_version to model_iteration. For the code guidelines, I will create a separate task to make a pass over the entire repo in the near future.

actors/ppo_atari_pz.py Outdated Show resolved Hide resolved
actors/ppo_atari_pz.py Outdated Show resolved Hide resolved
actors/ppo.py Outdated Show resolved Hide resolved
@wduguay-air wduguay-air merged commit 64d6e6d into main Apr 21, 2023
@wduguay-air wduguay-air deleted the 120-migrate-model-registry-v2 branch April 21, 2023 20:13
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.

Migrate to new Model Registry SDK
4 participants