Dkps

[bkj]

No description provided.

[bkj]

prior run-level DKPS prediction

[bkj]

instance level DKPS prediction

[bkj]

modification to allow subsampling of the training instances - I don't know whether this potentially causes downstream problems, because the other two train dataframes don't get modified to reflect this subset operation

[bkj]

env stuff, can remove if you want, but shouldn't break anything if people don't use pixi package manager.

[Erotemic]

I think its fine to add if it helps you, but is the maximum version necessary? I like to keep dependencies bounded strictly on the bottom, but open on the top whenever possible. This lets us know what a minimum working version is, and let's tests fail loudly if something does break upstream.

[Erotemic]

Suggested change

	"pyyaml>=6.0.2"
	]
	[project.optional-dependencies]
	tests = [
	"xdoctest>=1.2.0",
	"pytest>=8.1.1",
	"coverage>=7.3.0",
	"pytest-cov>=3.0.0",
	]
	optional = [
	"gcsfs>=2024.9.0",
	]
	dkps = [
	"dkps @ git+https://github.com/jataware/dkps"
	]
	all = [
	"aiq-magnet[optional]",
	"aiq-magnet[tests]",
	"aiq-magnet[dkps]",
	]
	docs = [
	"sphinx >= 4.3.2",
	"sphinx-autobuild >= 2021.3.14",
	"sphinx_rtd_theme >= 1.0.0",
	"sphinxcontrib-napoleon >= 0.7",
	"sphinx-autoapi >= 1.8.4",
	"Pygments >= 2.9.0",
	"myst_parser >= 0.16.1",
	"sphinx-reredirects >= 0.0.1",
	]

I'd like to move dkps to be an optional dependency.

It would also be nice to tag the git dependency with a specific commit id (or at least a version tag).

(I think the suggestion diff is not working right, but hopefully you get the idea of what I'm trying to go for)

[Erotemic]

Pro tip: putting python code inside a function makes it run faster: https://stackoverflow.com/questions/11241523/why-does-python-code-run-faster-in-a-function

I suggest using the pattern:

def main():
   ... # your stuff

if __name__ == '__main__':
    main()

This is a very minor detail, but its a fun piece of trivia I like telling people about.

[Erotemic]

Avoid global random seeds. It looks like you already pass one in via the CLI.

[Erotemic]

Optional: I'd like to suggest using the scriptconfig module instead of argparse. The following code would replace your argument parser exactly:

import scriptconfig as scfg

class DKPSPredictorConfig(scfg.DataConfig):
    """
    Config for DKPS
    """
    num_example_runs = scfg.Value(50, type=int, help='Number of training runs used by DKPS.')
    num_eval_samples = scfg.Value(4, type=int, help='Number of queries used by DKPS.')
    seed = scfg.Value(1, type=int, help='Random seed to use.')
    n_components_cmds = scfg.Value(8, type=int, help='Number of components used by DKPS.')

# Works exactly the same as `argparse.ArgumentParser.parse_args`
args = DKPSPredictorConfig.cli(strict=True)

This gives a nice declarative way of defining a CLI, and also opens the door to calling the code within Python via the exact same API (among other quality of life enhancements).

I'd also recommend adding the path to the suite directory as a configuration argument. This way you can hard code the default (great for research code), but it allows other people to set it to the appropriate value for their system.

Recently we've also changed it such that you don't need to pass the suite and the root dir as separate arguments, you just path a single path in.

So to get an exact replication of what you have you could do something like:

class DKPSPredictorConfig(scfg.DataConfig):
    """
    Config for DKPS
    """
    helm_suite_dir = scfg.Value('/Users/bjohnson/data/crfm-helm-public/lite/benchmark_output/runs/_all', help='Path to helm suite to run.')
    num_example_runs = scfg.Value(50, type=int, help='Number of training runs used by DKPS.')
    num_eval_samples = scfg.Value(4, type=int, help='Number of queries used by DKPS.')
    seed = scfg.Value(1, type=int, help='Random seed to use.')
    n_components_cmds = scfg.Value(8, type=int, help='Number of components used by DKPS.')

and then use:

predictor(args.helm_suite_dir)

However, to accommodate your request to make it easy to run over multiple suites, we might want to change that to "helm_run_paths", and have that be specified as either an explicit list of the exact run paths you want to work with, a pattern that matches them, or perhaps if you just specify the root dir, it picks up everything.

I'll think about how to best implement that change. Unless @dmjoy or @b-fenelon is looking into it. I'm wondering if this is any more complicated than just letting the code in prepare_all_dataframes gather data from runs over different suites?

[dmjoy]

@Erotemic thanks for the review. I should have mentioned earlier, I think this PR is largely just where Ben is parking his predictor implementation code for right now. I've already started a repo (not shared yet; but using roughly the directory / repo structure I posted about) for this code to live in. Nonetheless all good feedback, but likely not to be prioritized until after the demo next week.