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

+exclude functionality! #54

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

+exclude functionality! #54

wants to merge 11 commits into from

Conversation

taodav
Copy link

@taodav taodav commented Jun 14, 2023

As mentioned in #53, this PR is for the +exclude functionality. +exclude removes a subset of the runs generated by +args, by removing all runs that include arguments with the cross product of all +exclude hyperparams. So for example:

prelaunch +command test +jobname test +arg --test1 1 2 3 +arg --test2 4 5 6 +exclude --test1 1 2 +exclude --test2 5

Should exclude all jobs with arguments

  • --test1 1 --test2 5
  • --test1 2 --test2 5

I've also written a test according to this example, but am unsure of how to run the test...

@camall3n
Copy link
Owner

camall3n commented Jul 29, 2023

This looks useful and the code is clean. Nice job!

I'm willing to merge this in if you will also update the README to document how it works. I think a short description plus a worked example would suffice. The example should use real-sounding variable names, so it's clear intuitively what the use case is.

@camall3n
Copy link
Owner

Regarding how to run tests, currently it's python tests/test_prelaunch.py.

But I'm noticing that the prelaunch tests broke when I added the history subcommand, and I didn't notice because we never set up any CI actions.

@camall3n
Copy link
Owner

@taodav I don't like messing with the constants in this PR. We should discuss where the default .onager directory goes in a separate PR.

@taodav
Copy link
Author

taodav commented Aug 23, 2023

My bad! Forgot this PR was open and implemented it for my own use.

@camall3n
Copy link
Owner

Np. But let's finish it and merge at some point! :)

@camall3n
Copy link
Owner

Please remove your .idea folder

@taodav
Copy link
Author

taodav commented Aug 23, 2023

Just caught it, removed!

@camall3n
Copy link
Owner

Awesome. Can you provide a worked example in the readme as well?

@taodav
Copy link
Author

taodav commented Aug 23, 2023

Done! Also added titles for the types of arguments and options.

@camall3n camall3n self-requested a review August 24, 2023 01:46
Comment on lines +164 to +165
print(f"Prelaunched {len(jobs)} jobs for {args.jobname}.")

Copy link
Owner

Choose a reason for hiding this comment

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

Lol stop sneaking in other functionality! Does this work correctly with the +quiet argument?

Comment on lines +9 to +30
def filter_cmd_prefix(exclude_variables, cmd_prefix_list, VAR_SEP=' '):
exclude_arg_keys = []
for key, variable in exclude_variables.items():
curr_key_strs = []
for v in variable:
curr_key_strs.append(key + VAR_SEP + v)

exclude_arg_keys.append(curr_key_strs)

filtered_prefix_list = []
for cmd_prefix in cmd_prefix_list:
all_keys_match = True if exclude_arg_keys else False
for curr_key_strs in exclude_arg_keys:
all_keys_match &= any(key_str in cmd_prefix for key_str in curr_key_strs)

if not all_keys_match:
break

if not all_keys_match:
filtered_prefix_list.append(cmd_prefix)

return filtered_prefix_list
Copy link
Owner

Choose a reason for hiding this comment

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

Comments please :)

I'm confused about all_keys_match, because it seems like exclude_arg_keys could be truthy even if you only have one exclude variable.

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.

2 participants