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

#171 updating docker templates to fabric8 #188

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Conversation

Cho-William
Copy link
Contributor

@Cho-William Cho-William commented Jul 3, 2024

  • updated archetype and foundation-mda templates to leverage fabric8 docker-maven-plugin in lieu of orphedomos
  • sagemaker antora documentation was updated to reflect updated configuration of ECR registry URL
  • machine learning pipeline and notifications to set the ECR registry URL has been updated
  • fixed null pointer exception bug with check for onnx conversion generation (MachineLearningPipelineGenerator)
  • release notes draft updated noting potential breaking changes regarding loss of extra orphedomos configs
  • update the orphedomos to fabric8 migration to include the default skip configuration

IMPORTANT NOTE

Please note that this PR disables fabric8's docker builds as part of a default build for a downstream project:

  • This configuration is consistent with the previous orphedomos configuration, although not leveraging this skip behavior would have been preferred
  • Unfortunately, if a user were to generate a project and exclusively use mvn clean install rather than mvn clean generate-sources (which is how the archetype test script also does it) to create their project structure, the user may eventually encounter a broken build. Consider the following example:
    • The user begins generating a downstream project with only mvn clean install calls
    • The user's machine learning inference docker module is generated
    • The user builds again, and if the docker fabric8 build is not skipped, then that docker module will begin building
      • However, the generation for the machine learning inference pipeline step has not yet been generated at this point
    • Finally, the inference docker image expects a copy of ie the sdist requirements.txt from the inference pipeline step to be available in the docker build context/directory, but that ml pipeline inference step module has not yet been built yet
      • The reason why the inference pipeline step module has not yet been generated is because it is a sub-sub directory of the -pipelines module, and requires another generate-sources execution. Unfortunately, this source generation occurs after the aforementioned docker build.
      • Thus, previous archetype tests and downstream projects did not encounter this issue, as orphedomos docker builds were also disabled by default

Therefore, there are two options to remedy this issue for the archetype test script and downstream projects:

  1. Simply keep the skip configuration as part of the default fabric8 docker build configuration
  2. Update foundation-mda's generation logic to automatically include sub-sub step modules as declared child modules in their respective pipeline parent poms. Namely this applies to machine learning pipelines, and any other pipelines that house child modules.

Unfortunately, option 2 was not able to be incorporated in the scope of this ticket, however it is in the interest of the aiSSEMBLE team to eventually implement option 2 and revert the default skip behavior

@@ -100,11 +104,16 @@ extension-pkg-whitelist = "pydantic"
ignore-patterns = '.*pb2[\S]*.py'
```

### Upgrade Steps for Transitioning from Orphedomos to Fabric8
If any extra configurations were added to the `orphedomos-maven-plugin` in addition to the generated defaults, executing the migration **will result in loss of these extra configurations**. To facilitate the upgrade, the following steps should be taken:
1. Before executing the baton migration, it is recommended to ensure some form of version control is in place to preserve your existing `orphedomos-maven-plugin` configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like these bullets should be 1., 2., 3., instead of all 1's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is intentional. That way if we insert another point, we don't have to manually renumber. Markdown is smart enough to increment the numbers when the README is rendered

Copy link
Contributor

Choose a reason for hiding this comment

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

oo good to know!

@@ -84,6 +84,18 @@
<version>${archetypeVersion}</version>
<scope>provided</scope>
</dependency>
<dependency>
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: are we sure we need foundation-mda? It's not really a maven plugin, but rather a dependency that is fed to fermenter. I would have though that the normal reactor would pick it up for the cache without explicitly being set.

If it is that the build cache isn't picking up dependencies listed within plugins, I might just note that so it's easier to know what it is here in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is indeed the latter case, because after adding these two dependencies, it resolved the inconsistent behavior with the archetype tests. I will add a comment to contextualize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will also make sure the additions are not part of the plugins comment block

@@ -100,11 +104,16 @@ extension-pkg-whitelist = "pydantic"
ignore-patterns = '.*pb2[\S]*.py'
```

### Upgrade Steps for Transitioning from Orphedomos to Fabric8
Copy link
Contributor

Choose a reason for hiding this comment

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

A: Maybe "Upgrade Steps for Projects with Customized Orphedomos Configurations"? Usually we try to convey what conditions would cause the instructions to be relevant to a project in these titles.

 - updated archetype and foundation-mda templates to leverage
   fabric8 docker-maven-plugin in lieu of orphedomos
 - sagemaker antora documentation was updated to reflect
   updated configuration of ECR registry URL
 - machine learning pipeline and notifications to set the
   ECR registry URL has been updated
 - fixed null pointer exception bug with check for
   onnx conversion generation (MachineLearningPipelineGenerator)
 - release notes draft updated noting potential breaking
   changes regarding loss of extra orphedomos configs
 - added back skip config for templates and migration
 - added foundation-mda and foundation-upgrade for
   archetype tests dependencies workaround
@Cho-William Cho-William force-pushed the 171-docker-mda-fabric8 branch from 4f9e404 to 8bd5f14 Compare July 5, 2024 13:20
@Cho-William Cho-William merged commit 75f317f into dev Jul 5, 2024
@Cho-William Cho-William deleted the 171-docker-mda-fabric8 branch July 5, 2024 13:22
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