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

Move plugin repos into this main repo #325

Merged
merged 15 commits into from
Jun 21, 2024
Merged

Move plugin repos into this main repo #325

merged 15 commits into from
Jun 21, 2024

Conversation

CarlinLiao
Copy link
Collaborator

To close #323. Adds SPT graph plugins and includes them in the docker build and push Makefile recipes. I've verified that make build-docker-images recipe does not error and did a dry run make -n build-and-push-docker-images to verify that it's assembling the recipe as expected, but I haven't run it because I'm afraid of overwriting the images in Docker Hub, especially the plugin images that're in active use.

I looked into it and discovered that the submodule feature was added to git in v1.5.3, which released in 2007.

@CarlinLiao CarlinLiao added the refactor Code change preserving functionality label Jun 7, 2024
@CarlinLiao CarlinLiao requested a review from jimmymathews June 7, 2024 14:08
@CarlinLiao CarlinLiao self-assigned this Jun 7, 2024
@CarlinLiao
Copy link
Collaborator Author

I had some consternation when it came to naming directories. The plugins' source code is in plugin/ but the Dockerfiles are in build/plugins/. This makes sense to me since plugin/ contains both all the plugins' source code as well as template(s) that are not themselves a plugin, while build/plugins/ contains all the Dockerfiles for the actual plugins that're meant to be built, but I can see how it can be confusing seeing as I've already confused myself while writing the Makefile recipes.

Similarly, I added the graph-generation subdirectory to both plugin/ and build/plugins/ under the assumption that at some point in the feature we want to support other plugin archetypes, even though this made writing the Makefile recipes marginally more complicated. (I even made an unpushed commit flattening the structure before reversing it.) However, I've structured the Docker Hub repos and Makefile recipes in a way such that it's assumed that the plugin names cannot clash with submodules or plugin names from other plugin archetypes, since graph-generation isn't included in the name of the Docker Hub repo but every repo is prepended with /nadeemlab/spt-.

@CarlinLiao
Copy link
Collaborator Author

Added a quick hot fix for a mostly unrelated issue that unnecessarily prepended "cuda-" to the version number for plugins that require CUDA to begin with (i.e., graph-transformer). I'm surprised this wasn't caught earlier given that we've been running the graph-transformer workflow to test it before, which worries me I'm doing something wrong this time.

Graph processing plugins are to be made available as Docker images, built from a Dockerfile following the template provided in `Dockerfile`.

Each plugin should have the following commands available from anywhere in the Docker image:
* `spt-plugin-print-graph-request-configuration`, which prints to `stdout` the configuration file intended to be used by this plugin to fetch graphs from an SPT instance to use for model training. An empty configuration file and a shell script to do this is provided in this repo, as well as the command needed to make this available in the template `Dockerfile`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the Dockerfile uses an ADD instruction to place graph.config into the Docker image of the built plugin, e.g. the cg-gnn plugin. But this file is only a template (empty target_name). How are the contents of graph.config to be specified at runtime for different datasets using the same Docker image?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the in-built graph.config is not used, and a separate graph.config is meant to be supplied at runtime, then there doesn't seem to be much use for this command: spt-plugin-print-graph-request-configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot to update it when it says "this repo" to "the template" since this was originally the README for this template only. The graph.config file is not used within the Docker images, it's a (suggested default) setting file for the user to use with SPT when extracting the graph files to run the image(s) on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yes that is what I thought.
But then why is spt-plugin-print-graph-request-configuration used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's so you can get a graph.config file to use. It prints to stdout and then you copy it into your local directory and run spt graph extract with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the file it emits is never directly useable, right? If the idea is to manually edit the emitted template just before doing some run, it seems simpler to use the source code version of graph.config directly from this repository. The main runtime usage of the Docker container, to do its calculation, doesn't use the spt-plugin-print-graph-request-configuration command, if I understand correctly.

I think a similar remark applies to spt-plugin-print-training-configuration. What is the rationale for incorporating some of these development-specific actions into the docker image? Is it to avoid the need to access git source code in addition to a Docker image pull? If this is the case, we can perhaps make these 2:

  • spt-plugin-print-graph-request-configuration
  • spt-plugin-print-training-configuration

into commands provided by the template image only, and not indicated as "required" by the plugins themselves (since they are not required for the main action, training/processing).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put them in at your request a while back. I believe the rationale was to provide a way to get the recommended default settings without having to muck about with the Docker internals / source code.

The training variables cg-gnn wants are different from the training variables graph-transformer wants, and likewise the graph formats plugins want may be different from plugin to plugin (although in practice they're identical so far), so making it a template command would not work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, yeah this seems like the best that can be done now.

@CarlinLiao
Copy link
Collaborator Author

Through testing I discovered that the graph-transformer:0.0.1 Docker image was bugged, so I've uploaded a new one using the new Makefile recipe to fix it.

The errors were disqualifying (it lacked necessary apt packages to run cv2 and was not updated to switch to relative references when the script was moved from app/ to /usr/bin/local broke references), so I'm puzzled how we were able to run and upload graph-transformer results beforehand.

@jimmymathews jimmymathews merged commit bdaf0de into main Jun 21, 2024
1 check passed
@jimmymathews jimmymathews deleted the issue323 branch September 20, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code change preserving functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidating by bringing in spt plugin repository contents
2 participants