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

Add workflow to build ml-metadata #206

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

aktech
Copy link

@aktech aktech commented Sep 20, 2024

This PR creates an action to build ml-metadata package via docker.

  • Add build from docker
  • Upload to PyPi
  • Run tests via Pytest

Passing here on my fork: https://github.com/aktech/ml-metadata/actions/runs/11081764726/job/30793914107

Note:

  • Since the tests uses abseil, with class based tests, I have removed the class which it subclasses to unittest as there is better compatibility of pytest with unittest. Removing all the class based test wouldn't have been wise as that would require finding compatible equivalents and replacing all instances, which should better be done in parts instead of in one go, if the team decided to get completely rid of abseil.

  • Removed flags from abseil to use pytest addoption

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

We also need the part of the workflow that automatically uploads to PyPI when a release is made. See https://github.com/tensorflow/tfx/blob/master/.github/workflows/wheels.yml for an example of how to do this.

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Usually I like to split builds and testing because they're separate concerns, but in this case building takes a LONG time if I remember correctly so I could be convinced here that this is a better approach. The only other thing is that absltest.TestCase is a child class of unittest.TestCase, so I'd be interested to know if replacing absltest.TestCase is strictly necessary; otherwise I think we don't need that change. But I don't think this should block merging.

ml_metadata/metadata_store/mlmd_types_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

A few things remain:

  • revert unit test/absltest change
  • what do you think of putting pytest.ini content in pyproject.toml? It's a good place for project settings, and would cut down on the number of config files we have...

- move pytest.ini to pyproject.toml
- reusable steps instead
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@XinranTang
Copy link
Collaborator

Hi, could you resolve the conflicts in this branch?

@aktech
Copy link
Author

aktech commented Oct 10, 2024

Hi, could you resolve the conflicts in this branch?

Done, thanks!

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