Skip to content

buildtest config compilers find --file#1420

Merged
shahzebsiddiqui merged 24 commits intodevelfrom
writeAlternativePath
Apr 7, 2023
Merged

buildtest config compilers find --file#1420
shahzebsiddiqui merged 24 commits intodevelfrom
writeAlternativePath

Conversation

@Xiangs18
Copy link
Collaborator

@Xiangs18 Xiangs18 commented Mar 9, 2023

No description provided.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: -67.04 ⚠️

Comparison is base (cde6a68) 68.57% compared to head (3b65cb7) 1.54%.

❗ Current head 3b65cb7 differs from pull request most recent head a7c5745. Consider uploading reports for the commit a7c5745 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            devel   #1420       +/-   ##
==========================================
- Coverage   68.57%   1.54%   -67.04%     
==========================================
  Files          57      51        -6     
  Lines        6453    6312      -141     
  Branches     1158    1131       -27     
==========================================
- Hits         4425      97     -4328     
- Misses       2026    6199     +4173     
- Partials        2      16       +14     
Impacted Files Coverage Δ
buildtest/buildsystem/checks.py 0.00% <0.00%> (-5.61%) ⬇️
buildtest/cli/__init__.py 0.00% <0.00%> (-91.67%) ⬇️
buildtest/cli/compilers.py 0.00% <0.00%> (-27.59%) ⬇️
buildtest/cli/help.py 0.00% <0.00%> (-94.59%) ⬇️
buildtest/cli/info.py 0.00% <0.00%> (-87.10%) ⬇️
buildtest/cli/inspect.py 0.00% <ø> (-97.47%) ⬇️
buildtest/utils/file.py 10.39% <0.00%> (-77.11%) ⬇️

... and 50 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 13, 2023
@Xiangs18 Xiangs18 linked an issue Mar 13, 2023 that may be closed by this pull request
2 tasks
Comment on lines +1009 to +1014
compiler_find.add_argument(
"-f",
"--file",
action="store_true",
help="Wrtie configuration file to a new file",
)
Copy link
Member

Choose a reason for hiding this comment

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

can you please use parent parser for defining --file option so we can potentially use this option in other subcommands.

Copy link
Member

Choose a reason for hiding this comment

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

create it as a separate variable see

parent_parser = argparse.ArgumentParser(add_help=False)
parent_parser.add_argument(
"--pager", action="store_true", help="Enable PAGING when viewing result"
)
i think we need to have several parent parsers to help organize where options are inherited. I think we will likely see a clash with option -f so rule of thumb when using parent parser only have long option for now, later we can figure out which ones have short option such as -f

Comment on lines +1024 to +1028
# use parent parser for defining --file option
file_parser = argparse.ArgumentParser(parents=[parent_parser])
file_parser.add_argument(
"--file", action="store_true", help="Wrtie configuration file to a new file"
)
Copy link
Member

Choose a reason for hiding this comment

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

can you please define the file_parser next to

parent_parser = argparse.ArgumentParser(add_help=False)
I think we should probably have some variable self.parent_parsers as a list and then we can pass this around to each method where needed.

Have something like this

self.parent_parsers['file'] = argparse.ArgumentParser(add_help=False)
self.parent_parsers['file'].add_argument("--file", action="store_true", help="Wrtie configuration file to a new file")

Then you can add the parent parser in the following line

compiler_find = subparsers_compiler.add_parser(
"find",
help="Find compilers",
)

by doing

    compiler_find = subparsers_compiler.add_parser(
        "find",
        help="Find compilers",
        parents=[self.parent_parsers['file']]
    )

Does that make sense?.

Copy link
Collaborator Author

@Xiangs18 Xiangs18 Mar 22, 2023

Choose a reason for hiding this comment

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

I assume I will have the following in the get_parser function:

parent_parser = {}
parent_parser["pager"] = argparse.ArgumentParser(add_help=False)
parent_parser["pager"].add_argument(
        "--pager", action="store_true", help="Enable PAGING when viewing result"
    )
parent_parser["file"] = argparse.ArgumentParser(add_help=False)
parent_parser["file"].add_argument(
        "--file", action="store_true", help="Wrtie configuration file to a new file"

And then pass parent_parser around to each menu method where needed.

If my understanding is correct, only in compiler_find will parent = [self.parent_parsers['file']].
All others that used to have parents=[parent_parser] params will become parent = [self.parent_parsers["pager"]]?

Thank you!!

Copy link
Member

Choose a reason for hiding this comment

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

yes except change parent_parser to self.parent_parser that way we dont need to pass variable around each method. Yes i think you understand the logic.

Depending on the subcommand you will have parents=[self.parent_parsers['pager'] where --pager option is inherited. Likewise, where --file is inherited you can do parents=[self.parent_parsers['file']]. In event multiple arguments need to be inherited to a sub-command we should be able to index the dict so parents=[self.parent_parsers['pager'], [self.parent_parsers['file']].

Also a tip when copying code in markdown you can use triple backticks to format code. If you want to format python code you can do ```python. Github supports syntax highlighting for several languages you should take a look at https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-and-highlighting-code-blocks#syntax-highlighting

@Xiangs18
Copy link
Collaborator Author

Xiangs18 commented Apr 4, 2023

Hi @shahzebsiddiqui, running command below keeps getting unrecognized arguments error. Can you take a look?

buildtest config compilers find --file /tmp/buildtest.config.yml -u

usage: buildtest [options] [COMMANDS]
buildtest: error: unrecognized arguments: /tmp/buildtest.config.yml


parent_parser["file"] = argparse.ArgumentParser(add_help=False)
parent_parser["file"].add_argument(
"--file", action="store_true", help="Wrtie configuration file to a new file"
Copy link
Member

Choose a reason for hiding this comment

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

yes the issue is right here i missed this, please remove action="store_true" we need argparse to hold value of the file not a true or false. This should fix the issue

@shahzebsiddiqui
Copy link
Member

Hi @shahzebsiddiqui, running command below keeps getting unrecognized arguments error. Can you take a look?

buildtest config compilers find --file /tmp/buildtest.config.yml -u

usage: buildtest [options] [COMMANDS] buildtest: error: unrecognized arguments: /tmp/buildtest.config.yml

see my message https://github.com/buildtesters/buildtest/pull/1420/files#r1157445437 the add_argument is not setup correctly. Let me know if this fix. the issue

@shahzebsiddiqui
Copy link
Member

@Xiangs18 i saw the gitlab runner was stuck so i restarted it now your CI job is running on Perlmutter https://software.nersc.gov/NERSC/buildtest/-/jobs/202208 hopefully we will see the test coverage results in codecov.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 7, 2023
@Xiangs18
Copy link
Collaborator Author

Xiangs18 commented Apr 7, 2023

Screen Shot 2023-04-06 at 8 19 59 PM

@Xiangs18 Xiangs18 changed the title [WIP] buildtest config compilers find --file buildtest config compilers find --file Apr 7, 2023
@shahzebsiddiqui shahzebsiddiqui merged commit 7e3a249 into devel Apr 7, 2023
@shahzebsiddiqui shahzebsiddiqui deleted the writeAlternativePath branch April 7, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE]: buildtest config compilers find --file

2 participants