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 Dockerhub publishing step to the release GitHub Action workflow #740

Merged
merged 7 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# This workflow builds Python distribution packages using cibuildwheel tool and
# environment and publishes packages as a part of a GitHub release.
# environment and publishes packages as a part of a GitHub release. Also it
# releases container image to the DockerHub.

# This workflow uses actions that are not certified by GitHub. They are
# provided by a third-party and are governed by separate terms of service,
# privacy policy, and support documentation.

name: release python
name: release

on:
workflow_dispatch:
release:
types: [published]


jobs:
build-wheels:
name: Build wheels on ${{ matrix.os }}
Expand Down Expand Up @@ -77,35 +77,79 @@ jobs:
permissions:
id-token: write
environment:
name: test
name: pypi-test
runs-on: ubuntu-latest
needs: [build-wheels]
if: github.event.action == 'published'
steps:
- uses: actions/download-artifact@v4
with:
pattern: python-wheels-*
merge-multiple: true
path: dist
- name: Publish package distributions to Test PyPI
uses: pypa/gh-action-pypi-publish@release/v1
with:
repository-url: https://test.pypi.org/legacy/

- uses: actions/download-artifact@v4
with:
pattern: python-wheels-*
merge-multiple: true
path: dist

- name: Publish package distributions to Test PyPI
uses: pypa/gh-action-pypi-publish@release/v1
with:
repository-url: https://test.pypi.org/legacy/

publish-pypi:
name: Publish to PyPI
permissions:
id-token: write
environment:
name: production
name: pypi-production
runs-on: ubuntu-latest
needs: [build-wheels]
if: github.event.action == 'published'
steps:

- uses: actions/download-artifact@v4
with:
pattern: python-wheels-*
merge-multiple: true
path: dist

- name: Publish package distributions to PyPI
uses: pypa/gh-action-pypi-publish@release/v1

publish-docker:
name: Publish Docker image
environment:
name: dockerhub-production
runs-on: ubuntu-latest
needs: [build-wheels]
if: github.event.action == 'published'
steps:
- uses: actions/download-artifact@v4
with:
pattern: python-wheels-*
merge-multiple: true
path: dist
- name: Publish package distributions to PyPI
uses: pypa/gh-action-pypi-publish@release/v1

- name: Set up Docker BuildX
uses: docker/setup-buildx-action@v3

- name: Build and export to Docker
uses: docker/build-push-action@v6
with:
load: true
build-args: |
BUILDKIT_CONTEXT_KEEP_GIT_DIR=1
tags: trueagi/hyperon:test

- name: Test
run: |
echo "(* 7 6)" | docker run --rm trueagi/hyperon:test metta-repl

- name: Login to Docker Hub
uses: docker/login-action@v3
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

- name: Build and push
uses: docker/build-push-action@v6
with:
push: true
build-args: |
BUILDKIT_CONTEXT_KEEP_GIT_DIR=1
tags: |
trueagi/hyperon:${{github.event.release.tag_name}}
trueagi/hyperon:latest
11 changes: 8 additions & 3 deletions repl/src/metta_shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ pub mod metta_interface_mod {
}

pub fn confirm_hyperonpy_version(req_str: &str) -> Result<(), String> {

let req = parse_version_specifiers(req_str).unwrap();
let version_string = get_hyperonpy_version()?;
//NOTE: Version parsing errors will be encountered by users building hyperonpy from source with an abnormal configuration
Expand All @@ -78,7 +77,8 @@ pub mod metta_interface_mod {

match || -> Result<_, String> {
//Confirm the hyperonpy version is compatible
confirm_hyperonpy_version(">=0.1.0, <0.2.0")?;
let req_str = Self::required_hyperon_version();
confirm_hyperonpy_version(&req_str)?;

//Initialize the Hyperon environment
let new_shim = MettaShim::init_common_env(working_dir, include_paths)?;
Expand All @@ -93,6 +93,11 @@ pub mod metta_interface_mod {
}
}

fn required_hyperon_version() -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

I admit my hard-coded requirement was a bad solution and I am glad you are improving it.

I agree with what I think you're trying to do by using the repl's package.version; to reduce the number of configs we need to update and worry about when turning a release.

However, I don't like this approach of creating a VersionReq for HyperonPy by assuming it'll be within 1 minor version of the repl version.

One thought is to use a requirement string created from the actual hyperonpy version in CMake. This makes releasing easy, but it becomes annoying to use cargo directly to build the repl.

Another thought is to use a custom metadata key in the Cargo.toml. It's still two keys to update but they're in one place.

For example:

[workspace.package.metadata.hyperonpy]
hyperonpy-version-req = "==0.2.0"

Copy link
Contributor

Choose a reason for hiding this comment

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

UPDATE: After trying to implement the custom Cargo.toml metadata solution, it seems like a clumsy solution. It appears to require parsing the Cargo.toml file ourselves. (At lease if the way docs.rs does it is the recommended way. https://github.com/rust-lang/docs.rs/blob/7ad8c626bd63a95dc483143274ff6f2410a34af9/crates/metadata/lib.rs#L103)

My thinking is that, if a "correct" solution is clumsy and probably not necessary, we should go with a "mostly-works" solution that is as simple as possible.

The purpose of the check is to prevent version incompatibilities from showing up as mysterious bugs that we waste time chasing. So if we plan to keep the components within a release in version lock-step, then I don't think we need the semver range.

So how about something like this:

        fn required_hyperon_version() -> String {
            const PACKAGE_VERSION: &str = env!("CARGO_PKG_VERSION");
            format!("=={PACKAGE_VERSION}")
        }

Copy link
Collaborator Author

@vsbogd vsbogd Jul 12, 2024

Choose a reason for hiding this comment

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

Yes, I agree that if we split repo on different components: corelib, python lib, repl, then we need to specify repl dependency manually. And keeping it in metadata looks like a proper way of doing this. I didn't think about it on the first place because Python package version right now is formed from the GitHub tag and its major.minor.patch version in a common case is equal to the version of the repl. In my suggestion I was trying to reproduce the previous code behavior using repl package version. Strictly speaking we cannot just rely repl and hyperonpy versions are changed by the same rules (because standards are different) but on the other hand while we are changing them we can rely on this.

I agree that we can just require this versions to be equal while we have them in a same repo. This implementation should cover most of the cases. In practice dev Python package version always contains local version like +ga9d9ff1a.d20240712 but as PEP440 requires local version to be compatible with non local one it passes check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied your suggestion in 61ee39b

const PACKAGE_VERSION: &str = env!("CARGO_PKG_VERSION");
format!("=={PACKAGE_VERSION}")
}

pub fn init_common_env(working_dir: PathBuf, include_paths: Vec<PathBuf>) -> Result<MettaShim, String> {
match Python::with_gil(|py| -> PyResult<(Py<PyModule>, Py<PyAny>)> {
let py_mod = PyModule::from_code(py, Self::PY_CODE, "", "")?;
Expand Down Expand Up @@ -470,4 +475,4 @@ pub fn strip_quotes(src: &str) -> &str {
}
}
src
}
}
Loading