Added config parsing check to GaNDLF task runner

[sarthakpati]

No description provided.

[sarthakpati]

So, there are a few issues:

1. The current test uses GaNDLF master as its reference point [ref], which is not ideal, and we should change this to the latest tag, instead. This will ensure that we are not trying to hit a moving target with every merged commit into GaNDLF.

2. The current default GaNDLF config in the plan [ref] relies on an old version of the config instantiation [ref]. This has worked out thus far because GaNDLF API has been good with backwards compatibility (at least thus far). I would prefer to get this config from the latest tag's segmentation config that GaNDLF uses for tests, like this:

git clone --depth=1 https://github.com/MLCommons/GaNDLF.git
cd GaNDLF
git fetch --tags
echo "Checkout the latest GaNDLF tag"
latestTag=$(git describe --tags "$(git rev-list --tags --max-count=1)")
git checkout $latestTag
cat ./testing/config_segmentation.yaml  ### this is always tested in the GaNDLF CI

3. The default behavior of the parseConfig submodule of GaNDLF is to check for the version [ref], which can be used to circumvent my proposal above, but I would recommend against it since it would make the test brittle. Any future errors related to API changes in the config would be hard(er) to track down.

Thoughts, @psfoley?

[psfoley]

Thanks for the PR and detailing the issues, @sarthakpati. Am I understanding correctly that the change to load the default params in the PR is unpreferred? If so let's avoid that. I agree it's better to test against the latest tag and use the latest GaNDLF configuration format. To ensure OpenFL is always up to date with the latest GaNDLF format, maybe we could rely on this functionality:

fx plan initialize --gandlf_config ${PATH_TO_GANDLF_CONFIG}.yaml

To pull a GaNDLF reference configuration file from your repo that get's updated if/when there's a new tag with changes? Would it be possible to host and maintain this config file on the GaNDLF repo, or maybe there's one we can already use?

Crossing this out because it looks like GaNDLF/testing/config_segmentation.yaml is the right file for OpenFL to import

[sarthakpati]

Added these lines:

echo "Initialize OpenFL plan"
fx plan initialize --gandlf_config testing/config_segmentation.yaml

Getting this error [ref]:

Initialize OpenFL plan
Usage: fx plan initialize [OPTIONS]
Try 'fx plan initialize --help' for help.

Did I miss something?

[sarthakpati]

@psfoley: when I am trying to initialize the plan using the gandlf_config parameter [ref], the gandlf_config in the plan seems to be empty [ref]. Any idea what's happening?

[psfoley]

@psfoley: when I am trying to initialize the plan using the gandlf_config parameter [ref], the gandlf_config in the plan seems to be empty [ref]. Any idea what's happening?

There's a couple things going on here:

1. The path that you're importing the gandlf config from doesn't exist, and OpenFL is not throwing an error (I'll file a bug to add a check for this). I think if you change the command to the following the plan initialization should work correctly:

        fx plan initialize --gandlf_config ../testing/config_segmentation.yaml

2. You're initializing the plan, but then the final test that runs is:

python -m tests.github.test_gandlf --template gandlf_seg_test --fed_workspace aggregator --col1 one --col2 two --rounds-to-train 1

This will not use the plan you initialized with fx plan initialize ..., but the original gandlf_seg_test plan [ref] instead. It would probably be easiest to delay the plan initialization (with gandlf config) to this line [ref]

[sarthakpati]

2. You're initializing the plan, but then the final test that runs is:

python -m tests.github.test_gandlf --template gandlf_seg_test --fed_workspace aggregator --col1 one --col2 two --rounds-to-train 1

This will not use the plan you initialized with fx plan initialize ..., but the original gandlf_seg_test plan [ref] instead. It would probably be easiest to delay the plan initialization (with gandlf config) to this line [ref]

Thanks for the tips, @psfoley! I have incorporated this into the latest updates.

Unfortunately, it still doesn't seem to be working. Any ideas?

[sarthakpati]

@psfoley - this seems to be still taking the older version of the config. Can you please check to see what I am missing?

[sarthakpati]

@psfoley - this seems to be still taking the older version of the config. Can you please check to see what I am missing?

The new version of the config is corrected detected at the start [ref], but then it reverts to the older one [ref]. Could you please provide some insight on why this is the case?

[psfoley]

Tests are now passing 🚀 Thanks @sarthakpati! Approved and merged

[sarthakpati]

Thanks for all your help with this, @psfoley! 😄