1529075 - Use argparse to get local command line opts in Raptor's mozharness script

Status: RESOLVED WONTFIX

(Testing :: Raptor, enhancement, P3)

Description

[Robert Wood [:rwood]]

Currently the way Raptor retrieves command line options when running locally via mach (mozharness) is not the cleanest [1]. This should be improved by using argparse instead.

[1] https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/testing/mozharness/mozharness/mozilla/testing/raptor.py#182

Comment 1

[matthew.jay.wong]

Hi I love to be assigned this bug.

Comment 2

[matthew.jay.wong]

Attachment: This is a patch file for this bug

Please let me know if this is the right direction for this bug.

Comment 3

[matthew.jay.wong]

Please help to advice how to test/debug this part of mozilla.
Thanks

Comment 4

[Robert Wood [:rwood]]

(In reply to matthew.jay.wong from comment #3)

Please help to advice how to test/debug this part of mozilla.
Thanks

Hi Matthew, thanks for your interest in contributing to Raptor!

Since you've already submitted a patch I'm assuming you have a local Firefox dev environment already up and running. Here's the Raptor wiki so that you can learn more about Raptor and how to run it locally:

https://wiki.mozilla.org/Performance_sheriffing/Raptor

I'll have a look at your patch, thanks for submitting.

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 9045085 [details] [diff] [review]
This is a patch file for this bug

Review of attachment 9045085 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.

Note that your patch doesn't apply cleanly, as it looks like this is a diff after a change you had made previously.

-            # Bug fix 1529075
+
+            # Bug fix 1529075: Use argparse to read app when raptor is initiated locally

The top line doesn't exist in the original source, so the patch fails to be applied/merged.

I don't believe that you have tried this locally, as the patch doesn't run in it's current form - if you try to run it you'll see there is an 'unrecognized arguments' error. The parser is only looking for the 'app' argument, however it must support all the command line arguments as listed in the config_options (line 52). You're on the right track, after you fix please test locally, running the command line as noted in the Raptor wiki (comment 4), and then submit again.

Please only submit for either feedback -or- review, we use feedback if you would like some preliminary input (like this), and we use review for when the patch is working and tested locally and ready for review.

Thanks! :)

Comment 6

[matthew.jay.wong]

(In reply to Robert Wood [:rwood] from comment #4)

(In reply to matthew.jay.wong from comment #3)

Please help to advice how to test/debug this part of mozilla.
Thanks

Hi Matthew, thanks for your interest in contributing to Raptor!

Since you've already submitted a patch I'm assuming you have a local Firefox dev environment already up and running. Here's the Raptor wiki so that you can learn more about Raptor and how to run it locally:

https://wiki.mozilla.org/Performance_sheriffing/Raptor

I'll have a look at your patch, thanks for submitting.

Thanks Robert! I'll be sure to take a look at the Raptor docs.

Comment 7

[matthew.jay.wong]

(In reply to Robert Wood [:rwood] from comment #5)

Comment on attachment 9045085 [details] [diff] [review]
This is a patch file for this bug

Review of attachment 9045085 [details] [diff] [review]:

Thanks for the patch.

Note that your patch doesn't apply cleanly, as it looks like this is a diff
after a change you had made previously.

•        # Bug fix 1529075
  

•        # Bug fix 1529075: Use argparse to read app when raptor is
  

initiated locally

The top line doesn't exist in the original source, so the patch fails to be
applied/merged.

I don't believe that you have tried this locally, as the patch doesn't run
in it's current form - if you try to run it you'll see there is an
'unrecognized arguments' error. The parser is only looking for the 'app'
argument, however it must support all the command line arguments as listed
in the config_options (line 52). You're on the right track, after you fix
please test locally, running the command line as noted in the Raptor wiki
(comment 4), and then submit again.

Please only submit for either feedback -or- review, we use feedback if you
would like some preliminary input (like this), and we use review for when
the patch is working and tested locally and ready for review.

Thanks! :)

Thanks for taking a look at my initial patch. I'll make sure to submit for feedback or review only next time!

Comment 8

[matthew.jay.wong]

Attachment: This is a patch file for Bug: 1529075

Hey Robert,

So from the original problem description, you mentioned that retrieving command line options when running locally via mach is not the "cleanest". But later on in the feedback you gave in my initial patch mentioned that I need to accommodate all arguments in the config_options (line 52).

I'm wondering in terms of the scope of code changes that need to be made, should I keep it to within the if statement (lines 176-187) or fix the rest of the parts where the raptor_cmd_line_args are used in lines 202, 379, 73.

Please also let me know if there's anything else that can be improved!

Thanks,
Matt

Comment 9

[Robert Wood [:rwood]]

Hi Matt, first step: Do you have a local development environment all setup and building Firefox successfully (via mach bootstrap and mach build)? Secondly, are you able to run Raptor within your dev repo (using mach raptor-test as noted in the wiki)? Thanks!

Comment 10

[matthew.jay.wong]

(In reply to Robert Wood [:rwood] from comment #9)

Hi Matt, first step: Do you have a local development environment all setup and building Firefox successfully (via mach bootstrap and mach build)? Secondly, are you able to run Raptor within your dev repo (using mach raptor-test as noted in the wiki)? Thanks!

Hey Robert,

Yes I have a local development environment all setup with Firefox built successfully via mach bootstrap and mach build. Also I've been able to run Raptor locally within my dev repo with the different raptor-test(s).

As shown in my patch, it seems like I can parse the raptor arguments separately but I just want to know if the scope of the patch fix is constrained to just when raptor is run locally or to the other possible arguments for Raptor as you mentioned with the config_options (line 52).

Thanks,
Matt

Comment 11

[Robert Wood [:rwood]]

(In reply to matthew.jay.wong from comment #10)

As shown in my patch, it seems like I can parse the raptor arguments separately but I just want to know if the scope of the patch fix is constrained to just when raptor is run locally or to the other possible arguments for Raptor as you mentioned with the config_options (line 52).

So if you run Raptor with your first original patch you'll see it fails with an 'unrecognized arguments error' because there are more command line arguments besides 'app' provided on the command line. At this particular part of the code we are parsing out the 'app' argument; however you can't break Raptor in that it should still support the other arguments too.

In your latest patch I wouldn't re-define all the arguments (i.e. raptor_args_parser.add_argument('--test', help='Raptor test to run') because they are already all defined right above in the config. Maybe you can find a way to call add_argument but pass in the config above?

Hope that helps. :)

Comment 12

[matthew.jay.wong]

(In reply to Robert Wood [:rwood] from comment #11)

(In reply to matthew.jay.wong from comment #10)

As shown in my patch, it seems like I can parse the raptor arguments separately but I just want to know if the scope of the patch fix is constrained to just when raptor is run locally or to the other possible arguments for Raptor as you mentioned with the config_options (line 52).

So if you run Raptor with your first original patch you'll see it fails with an 'unrecognized arguments error' because there are more command line arguments besides 'app' provided on the command line. At this particular part of the code we are parsing out the 'app' argument; however you can't break Raptor in that it should still support the other arguments too.

In your latest patch I wouldn't re-define all the arguments (i.e. raptor_args_parser.add_argument('--test', help='Raptor test to run') because they are already all defined right above in the config. Maybe you can find a way to call add_argument but pass in the config above?

Hope that helps. :)

Hey Robert,

I see what you mean now!
Let me take another look at it and I will get back to you if I need more clarification.

Thanks,
Matt

Comment 13

[matthew.jay.wong]

Attachment: This is a patch file for Bug: 1529075

Hey Robert,

I took your advice and found a way to add all the arguments in the config_options[] to the raptor_arg_parser. But while doing so I found a few errors in the arguments in config_options[] as well as the other config options in testing_config_options[].

This includes line 95 in testbase.py that specifies type choice when argparse interprets type str, not type choice. Another example is that they use action extend when argparse uses action append. Lastly, types need to passed into add_argument without quotes which is why i had to strip the quotes from "int".

Please help to check and advice!
Thanks,
Matt

Comment 14

[Robert Wood [:rwood]]

Comment on attachment 9047507 [details] [diff] [review]
This is a patch file for Bug: 1529075

Hi Matthew,

Thanks - however no, this is not really what I had in mind here. After looking at this closer, I don't think fixing this issue will be worth the risk of breaking Raptor in production. Changing how we use the arguments just for the parsing of one argument is not really worthwhile if there's no easy fix. Therefore I'm going to close out this issue and we will leave the code as/is.

Thanks for your effort in this anyway. Feel free to pick up a different 'good-first-bug' if you wish to contribute.

Comment 15

[matthew.jay.wong]

(In reply to Robert Wood [:rwood] from comment #14)

Comment on attachment 9047507 [details] [diff] [review]
This is a patch file for Bug: 1529075

Hi Matthew,

Thanks - however no, this is not really what I had in mind here. After
looking at this closer, I don't think fixing this issue will be worth the
risk of breaking Raptor in production. Changing how we use the arguments
just for the parsing of one argument is not really worthwhile if there's no
easy fix. Therefore I'm going to close out this issue and we will leave the
code as/is.

Thanks for your effort in this anyway. Feel free to pick up a different
'good-first-bug' if you wish to contribute.

Hey Robert,

Thanks for following up on my suggestion.
Yes, I see your point and I also agree that leaving the code as is would be the best option.
I will be sure to go find another 'good first bug' to work on.

Thanks,
Matt