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

robot: fix Python client import #240

Merged
merged 2 commits into from
Dec 13, 2023
Merged

robot: fix Python client import #240

merged 2 commits into from
Dec 13, 2023

Conversation

isinyaaa
Copy link
Contributor

@isinyaaa isinyaaa commented Dec 12, 2023

This fixes Robot tests for the Python client.

Description

This fixes the imported python client on Robot tests to match what's exposed after adding a higher-level API.

How Has This Been Tested?

I haven't managed to run robot locally successfully yet, but it seems the import problem has been fixed.

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@isinyaaa isinyaaa requested a review from tarilabs December 12, 2023 23:33
@isinyaaa isinyaaa self-assigned this Dec 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b07bace) 74.52% compared to head (89ede4a) 74.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #240   +/-   ##
=======================================
  Coverage   74.52%   74.52%           
=======================================
  Files          17       17           
  Lines        2296     2296           
  Branches       73       73           
=======================================
  Hits         1711     1711           
  Misses        420      420           
  Partials      165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@isinyaaa
Copy link
Contributor Author

@tarilabs I haven't been able to run this locally per the instructions you gave me:

docker run -p 9090:8080 --env METADATA_STORE_SERVER_CONFIG_FILE=/tmp/shared/conn_config.pb --volume../test/bdd:/tmp/shared gcr.io/tfx-oss-public/ml_metadata_store_server:1.14.0

then

make build
./model-registry proxy

then

TEST_MODE=Python robot test

Am I doing something wrong? Is there a better way to run this?
From what I've noticed this fails because the MLMD types haven't been created, if so, I might be missing some step of the setup.

@tarilabs
Copy link
Member

Am I doing something wrong? Is there a better way to run this?
From what I've noticed this fails because the MLMD types haven't been created

When the proxy connects, it automatically creates the MLMD _type(s).
Do you get in the proxy the log line which shows successfull connection to MLMD gRPC?
If you connect to the SQLite db file, do you see the entries?

Copy link
Contributor

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

@isinyaaa I think the issue is in your volume, you are passing /test/bdd but you should pass the local dir where conn_config.pb is stored which is test/config/ml-metadata/, let's try with this change and it should work.

I tried locally and robot tests are working properly, therefore LGTM!!

@lampajr lampajr self-requested a review December 13, 2023 10:39
@lampajr
Copy link
Contributor

lampajr commented Dec 13, 2023

I tried locally and robot tests are working properly, therefore LGTM!!

Nevermind, I did not use the proper binary.. they are not working running: TEST_MODE=python robot test/robot

Copy link
Contributor

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

This should fix the issue for MRandLogicalModel.robot, still checking the other one

test/robot/ModelRegistry.py Outdated Show resolved Hide resolved
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
@isinyaaa isinyaaa requested a review from lampajr December 13, 2023 14:06
Copy link
Contributor

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

LGTM, robot tests are working for me

Copy link
Member

@tarilabs tarilabs 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 with me.

Made sure to have installed: pip install robotframework robotframework-requests PyYAML
and of course all python MR client dep and related.

Tested locally with.

In one terminal

docker run -p 9090:8080 --env METADATA_STORE_SERVER_CONFIG_FILE=/tmp/shared/conn_config.pb --volume ./test/config/ml-metadata:/tmp/shared gcr.io/tfx-oss-public/ml_metadata_store_server:1.14.0

In another terminal

./model-registry proxy
I1213 18:35:25.649365   14668 proxy.go:31] proxy server started at localhost:8080
I1213 18:35:25.649665   14668 proxy.go:37] connecting to MLMD server localhost:9090..
I1213 18:35:25.678205   14668 proxy.go:49] connected to MLMD server

In a third terminal:

robot UserStory.robot
TEST_MODE=Python robot MRandLogicalModel.robot
robot MRandLogicalModel.robot

image

@isinyaaa isinyaaa merged commit 077be7c into opendatahub-io:main Dec 13, 2023
7 checks passed
@isinyaaa isinyaaa deleted the fix-robot-py branch December 13, 2023 20:35
@isinyaaa
Copy link
Contributor Author

thanks a lot for the instructions, Matteo :) I managed to run it as well

@tarilabs
Copy link
Member

thanks a lot for the instructions, Matteo :) I managed to run it as well

Always glad to help @isinyaaa , teamwork to the moon 🚀 🌙

dhirajsb pushed a commit to dhirajsb/model-registry that referenced this pull request Jan 20, 2025
…pendatahub-io#240)

Bumps [black](https://github.com/psf/black) from 24.4.2 to 24.8.0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@24.4.2...24.8.0)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

4 participants