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

Add custom argparse actions #155

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Conversation

Pennycook
Copy link
Contributor

Related issues

  • Part of Support custom compiler arguments #145; these two actions are required to configure argparse such that its behavior is equivalent to the custom argument handling code introduced in CBI 1.2.0.

Proposed changes

  • Add _StoreSplitAction to handle comma-delimited arguments (e.g., -fsycl-targets=t1,t2).
  • Add _ExtendMatchAction to handle arguments requiring regular expressions (e.g., --gencode=sm_70,compute_75).
  • Add tests for both new actions.

Recognizing custom compiler arguments is a big feature, so I'm going to try and split it out into bite-sized chunks for review.

Handles flags of the form "--foo=one,two", which are commonly used to describe
compilation targets (e.g., "-fsycl-targets=spir64,nvptx64-nvidia-cuda").

Signed-off-by: John Pennycook <[email protected]>
Handles more complex flags of the form "--foo=one,two", where the important
information is embedded in another string matching some known pattern
(e.g., in "--gencode=sm_60,compute_60", "60" is the important information).

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added the enhancement New feature or request label Jan 22, 2025
@Pennycook Pennycook added this to the 2.0.0 milestone Jan 22, 2025
@Pennycook Pennycook requested a review from laserkelvin January 22, 2025 12:11
Copy link
Contributor

@laserkelvin laserkelvin left a comment

Choose a reason for hiding this comment

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

Just general comment - not sure if you want to add type annotations to the function signatures: even though the classes are private, would make it easier to maintain if you can glean argument types from a glance

@Pennycook
Copy link
Contributor Author

Just general comment - not sure if you want to add type annotations to the function signatures: even though the classes are private, would make it easier to maintain if you can glean argument types from a glance

Good idea. I've done this in 1430ab6. I didn't add annotations for kwargs, mainly because I don't know what they should be -- we pass the kwargs along to argparse, and the remaining arguments can be pretty much anything. I think the fact the argument is called kwargs explains what it is, though.

@Pennycook
Copy link
Contributor Author

The CI reports the following lines as being uncovered:

6 uncovered lines in codebasin/__main__.py: 271,272,327,335,341,354
1 uncovered lines in codebasin/config.py: 280
1 uncovered lines in codebasin/preprocessor.py: 691
19 uncovered lines in codebasin/report.py: 46,47,49,50,175,205,227,258,293,294,296,357,358,368,369,795,796,800,805

None of these lines were actually touched by this PR, so the action is being a little overzealous. I'm going to ignore it and merge anyway, but I need to take another look at why the coverage action keeps generating the wrong list of commits.

@Pennycook Pennycook merged commit 7ee2e29 into intel:main Feb 4, 2025
3 of 4 checks passed
@Pennycook Pennycook deleted the custom-argparse-actions branch February 4, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants