Enable building specified components #265

PhilippeMoussalli · 2023-07-04T13:14:43Z

PR that enables specifying components during the build and tagging process, this makes it easier to build and push specific components during remote development.

This reverts commit dee7044.

This reverts commit 17f9d97.

RobbeSneyders

Thanks @PhilippeMoussalli

I would like to be able to build the build_components.sh script for example components as well. So I would propose to add the following arguments:

-d, --components-dir directory with components to build, defaults to components
-co, --component specific component to build in the components dir, defaults to all

Note that currently we are passing in the first tag as the fondant version as well, which is used as pip install git+https://github.com/ml6team/fondant@$tag. So this will not work with arbitrary tags. They actually need to be refs available on the github repository. This script was made for usage in CI/CD and I'm not sure how well it will work outside it.

scripts/build_components.sh

PhilippeMoussalli · 2023-07-04T15:19:32Z

Thanks @PhilippeMoussalli

I would like to be able to build the build_components.sh script for example components as well. So I would propose to add the following arguments:

-d, --components-dir directory with components to build, defaults to components

-co, --component specific component to build in the components dir, defaults to all

Note that currently we are passing in the first tag as the fondant version as well, which is used as pip install git+https://github.com/ml6team/fondant@$tag. So this will not work with arbitrary tags. They actually need to be refs available on the github repository. This script was made for usage in CI/CD and I'm not sure how well it will work outside it.

I can add the component directory argument in this PR. Regarding the other issue, I am still tagging them with the latest release tag and then using the tag_component script to give them a desired tag and test them out (dev or latest). It's fine for now for testing but perhaps we should later on look into building directly from the current source code.

RobbeSneyders · 2023-07-04T15:25:26Z

I can add the component directory argument in this PR. Regarding the other issue, I am still tagging them with the latest release tag and then using the tag_component script to give them a desired tag and test them out (dev or latest). It's fine for now for testing but perhaps we should later on look into building directly from the current source code.

Not sure if I understand what you mean with this, but outside of the CI/CD pipeline, we should never tag images with:

release versions
dev
latest

As these have special meaning in our release flow.

PhilippeMoussalli · 2023-07-05T07:29:38Z

I can add the component directory argument in this PR. Regarding the other issue, I am still tagging them with the latest release tag and then using the tag_component script to give them a desired tag and test them out (dev or latest). It's fine for now for testing but perhaps we should later on look into building directly from the current source code.

Not sure if I understand what you mean with this, but outside of the CI/CD pipeline, we should never tag images with:

release versions

dev

latest

As these have special meaning in our release flow.

Updated to add the directory as an argument to enable building custom images.

I see, so if we want to avoid tagging image with the release_version we have to uncouple the fact that the tag is used to specify the fondant version to build and also specifies the image tag. For development before merging to main, we can give arbitrary tags (e.g. dev_philippe)

RobbeSneyders · 2023-07-05T12:39:59Z

I don't think arbitrary tags will work, but (remote) commit hashes will.

Eg. if you run ./scripts/build_components.sh -t d23e23, it will:

Tag images with d23e23
Pin the fondant dependency to git+https://github.com/ml6team/fondant@d23e23
Update fondant_component.yaml to use the d23e23 image

PhilippeMoussalli · 2023-07-05T12:55:25Z

I don't think arbitrary tags will work, but (remote) commit hashes will.

Eg. if you run ./scripts/build_components.sh -t d23e23, it will:

Tag images with d23e23

Pin the fondant dependency to git+https://github.com/ml6team/fondant@d23e23

Update fondant_component.yaml to use the d23e23 image

yes, the tagging I mentioned was mainly related to running the tag_component.sh script but I think this one is mainly reserved for releases to tag with the latest tag. In that case, we can still use the build_components.sh script for:

developing before merging to main (using the commit as the tag) -> requires running the script manually
start developing on the current main branch before release (using the dev as the tag) -> handled by CI/CD upon merge to main
using the latest images after the release (using the latest or release tag) -> handled by CI/CD upon release

Do you agree with this workflow? Anything else that needs to be done in this PR?

RobbeSneyders

Sorry for the nits on the help message, but I want to make sure we are talking about the same things :)

RobbeSneyders · 2023-07-05T12:41:11Z

scripts/build_components.sh

-  echo "  -t, --tag=<value>        Tag to add to image, repeatable
-                                   The first tag is set in the component specifications"
-  echo "  -h, --help               Display this help message"
+  echo "  -t,  --tag=<value>                 Tag to add to image, repeatable


Suggested change

echo " -t, --tag=<value> Tag to add to image, repeatable

echo " -t, --tag <value> Tag to add to image, repeatable

I don't think we handle = correctly. This was already documented incorrectly in the past.

RobbeSneyders · 2023-07-05T12:43:11Z

scripts/build_components.sh

+  echo "  -t,  --tag=<value>                 Tag to add to image, repeatable
+                                             The first tag is set in the component specifications"
+  echo "  -c,  --cache <value>               Use registry caching when building the components (default:false)"
+  echo "  -d,  --component-dirs <value>      Directory where the component is built from the root directory (default:components)"


Suggested change

echo " -d, --component-dirs <value> Directory where the component is built from the root directory (default:components)"

echo " -d, --component-dirs <value> Directory containing components to build as subdirectories. The path should be relative to the root directory (default:components)"

RobbeSneyders · 2023-07-05T12:44:33Z

scripts/build_components.sh

+                                             The first tag is set in the component specifications"
+  echo "  -c,  --cache <value>               Use registry caching when building the components (default:false)"
+  echo "  -d,  --component-dirs <value>      Directory where the component is built from the root directory (default:components)"
+  echo "  -n, --namespace <value>            Set the namespace (default: ml6team)"


Suggested change

echo " -n, --namespace <value> Set the namespace (default: ml6team)"

echo " -n, --namespace <value> The namespace for the built images, should match the github organization (default: ml6team)"

RobbeSneyders · 2023-07-05T12:46:31Z

scripts/build_components.sh

+  -d |--components-dir ) components_dir="$2"; shift;;
+  -r |--repo) repo="$2"; shift;;
+  -t |--tag) tags+=("$2"); shift;;
+  -co|--component) component="$2"; shift;;


Should we make this repeatable so you can specify multiple components?

RobbeSneyders · 2023-07-05T12:55:39Z

scripts/build_components.sh

+  echo "  -co, --component <value>           Set the component name. Pass the component folder name to build
+                                             a certain components or 'all' to build all components in the components
+                                             directory (default: all)"


Suggested change

echo " -co, --component <value> Set the component name. Pass the component folder name to build

a certain components or 'all' to build all components in the components

directory (default: all)"

echo " -co, --component <value> Specific component to build. Pass the component subdirecotry name

to build a certain components or 'all' to build all components in the components

directory (default: all)"

RobbeSneyders · 2023-07-05T13:01:10Z

scripts/build_components.sh

  pushd "$dir"

  BASENAME=${dir%/}
  BASENAME=${BASENAME##*/}
+  if [[ "$BASENAME" == "${component}" ]] || [[ "${component}" == "all" ]]; then


Can we do this a bit different? Python pseudo code:

components = [dir in components_dir] if component == "all" else [component] print(f"building components {components}") for component in components: ...

I think this would be easier to understand and debug.

RobbeSneyders

Thanks @PhilippeMoussalli! Looks good to me apart from some formatting issues.

RobbeSneyders · 2023-07-05T16:13:02Z

scripts/build_components.sh

@@ -75,5 +99,6 @@ for dir in "$component_dir"/*/; do
   --label org.opencontainers.image.source=https://github.com/${namespace}/${repo} \
   .

-  popd
+   popd


Wrong indentation

RobbeSneyders · 2023-07-05T16:13:05Z

scripts/build_components.sh

    cache_name=ghcr.io/${namespace}/${BASENAME}:build-cache
-    echo "Caching from/to ${cache_name}"
+     echo "Caching from/to ${cache_name}"


Wrong indentation

RobbeSneyders · 2023-07-05T16:13:11Z

scripts/build_components.sh

@@ -61,10 +84,11 @@ for dir in "$component_dir"/*/; do
    args+=(-t "$tag")
  done

-  # Add cache arguments if caching is enabled
+#  # Add cache arguments if caching is enabled


Double comment

RobbeSneyders · 2023-07-05T16:13:51Z

scripts/build_components.sh

  *) echo "Unknown parameter passed: $1"; exit 1;;
 esac; shift; done

 # Check for required argument
-if [ -z "${tags}" ]; then
+if [ ${#tags[@]} -eq 0 ]; then


Why was this needed?

both achieve the same purpose but was getting suggestion to change it in my IDE so I opted for checking if the array length is zero instead. I'll revert it back.

My question was mainly meant to learn, not to ask you to revert 😅
But if they're equivalent, both are fine for me.

RobbeSneyders · 2023-07-06T07:48:50Z

Seems like you have a conflict @PhilippeMoussalli. Feel free to merge after you resolve it.

PR that enables specifying components during the build and tagging process, this makes it easier to build and push specific components during remote development.

PhilippeMoussalli added 8 commits July 3, 2023 14:12

add clip url as an argument

dee7044

handle nested data type in HF writer

17f9d97

Merge branch 'main' of github.com:ml6team/fondant

2d5b82a

Merge branch 'main' of github.com:ml6team/fondant

f27501b

Merge branch 'main' of github.com:ml6team/fondant

01f7a31

enable-building-specific-reusable-components

1bd145d

Revert "add clip url as an argument"

fd99bd0

This reverts commit dee7044.

Revert "handle nested data type in HF writer"

bab38c7

This reverts commit 17f9d97.

PhilippeMoussalli requested review from RobbeSneyders and GeorgesLorre July 4, 2023 13:14

RobbeSneyders reviewed Jul 4, 2023

View reviewed changes

scripts/build_components.sh Outdated Show resolved Hide resolved

make build_images.sh applicable to custom images

c08a6d8

PhilippeMoussalli force-pushed the enable-building-specified-components branch from bcd4fca to c08a6d8 Compare July 5, 2023 07:20

RobbeSneyders reviewed Jul 5, 2023

View reviewed changes

PhilippeMoussalli added 3 commits July 5, 2023 16:20

correct docs

233e4f9

enable defining multiple components

a09ad82

Merge branch 'main' into enable-building-specified-components

4fb36c5

RobbeSneyders reviewed Jul 5, 2023

View reviewed changes

RobbeSneyders linked an issue Jul 5, 2023 that may be closed by this pull request

Make build_components.sh script generic for example components #245

Closed

correct indentation

579259b

RobbeSneyders approved these changes Jul 6, 2023

View reviewed changes

Merge branch 'main' into enable-building-specified-components

0911a73

PhilippeMoussalli merged commit bc5b7ce into main Jul 6, 2023

PhilippeMoussalli deleted the enable-building-specified-components branch July 6, 2023 07:51

Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023

Enable building specified components (#265)

5fe7e13

PR that enables specifying components during the build and tagging process, this makes it easier to build and push specific components during remote development.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable building specified components #265

Enable building specified components #265

PhilippeMoussalli commented Jul 4, 2023

RobbeSneyders left a comment

PhilippeMoussalli commented Jul 4, 2023

RobbeSneyders commented Jul 4, 2023

PhilippeMoussalli commented Jul 5, 2023 •

edited

Loading

RobbeSneyders commented Jul 5, 2023

PhilippeMoussalli commented Jul 5, 2023

RobbeSneyders left a comment

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders left a comment

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

RobbeSneyders Jul 5, 2023

PhilippeMoussalli Jul 6, 2023 •

edited

Loading

RobbeSneyders Jul 6, 2023

RobbeSneyders commented Jul 6, 2023

	echo " -t, --tag=<value> Tag to add to image, repeatable
	echo " -t, --tag <value> Tag to add to image, repeatable

	echo " -d, --component-dirs <value> Directory where the component is built from the root directory (default:components)"
	echo " -d, --component-dirs <value> Directory containing components to build as subdirectories. The path should be relative to the root directory (default:components)"

	echo " -n, --namespace <value> Set the namespace (default: ml6team)"
	echo " -n, --namespace <value> The namespace for the built images, should match the github organization (default: ml6team)"

Enable building specified components #265

Enable building specified components #265

Conversation

PhilippeMoussalli commented Jul 4, 2023

RobbeSneyders left a comment

Choose a reason for hiding this comment

PhilippeMoussalli commented Jul 4, 2023

RobbeSneyders commented Jul 4, 2023

PhilippeMoussalli commented Jul 5, 2023 • edited Loading

RobbeSneyders commented Jul 5, 2023

PhilippeMoussalli commented Jul 5, 2023

RobbeSneyders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeMoussalli Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbeSneyders commented Jul 6, 2023

PhilippeMoussalli commented Jul 5, 2023 •

edited

Loading

PhilippeMoussalli Jul 6, 2023 •

edited

Loading