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

Remove deprecated reparse_args usage #199

Merged
merged 5 commits into from
Aug 23, 2021
Merged

Conversation

BrianJKoopman
Copy link
Member

Description

site_config.reparse_args() has been deprecated for a while now, but is still very widely used in socs Agents. This removes it from the only core agent still using it, from one of the examples, and perhaps mostly importantly from the Agent developer documentation.

The one place I haven't removed it from is from ocsbow. I was finding in removing it from some socs Agents that if the Agent still called this line:

parser = site_config.add_arguments()

to initialize the parser, when site_config.parse_args() was later called (which internally calls site_config.add_arguments()) we get a conflict error from argparse. Maybe we can discuss how this should be handled here? This PR at least updates examples that might get copied as new Agents are developed. Then an issue can be created if we don't end up handling how add_arguments() interacts with things.

Motivation and Context

It's common for new Agents to start from copies of older Agents. This results in the deprecated reparse_args being used regularly in new code. Just trying to get things up to date.

How Has This Been Tested?

I've tested that style of change on other Agents, though really I should test these Agents too. I will open this as draft until those are done (likely early next week.)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Unless I am preparing a release, I have opened this PR onto the develop branch.

@BrianJKoopman BrianJKoopman requested a review from mhasself April 2, 2021 22:06
@mhasself
Copy link
Member

mhasself commented Apr 5, 2021

The example code that you fixed up is a little problematic still because it doesn't add any agent-specific command-line args. (--port would be a good one for demo.)

The documentation in https://ocs.readthedocs.io/en/develop/developer/site_config.html#agent-site-related-command-line-parameters is pretty good, but it is not linked from the Developers:Agents section... it should be.

The now-deprecated sequence is:

    site_parser = site_config.add_arguments()        # Get OCS base argparser
    parser = make_parser(site_parser)                # Add some Agent-specific args
    args = parser.parse_args()                       # Run the parser magic
    site_config.reparse_args(args, 'LabJackAgent')   # Fill in missing stuff with OCS rules

and that should be pretty close to the same thing as:

parser = make_parser()
args = site_config.parse_args(agent_class='LabJackAgent', parser=parser)

(Assuming of course that make_parser() can indeed make its own parser; otherwise pass in a new ArgumentParser().)

Were there SOCS agents using it in a weird way?

site_config.parse_args() seems suitable for agents but it is a little problematic for non-agents, such as clients and ocsbow... I think I have a fix for ocsbow at least; let me know if I should add it here or submit separately.

@BrianJKoopman
Copy link
Member Author

The example code that you fixed up is a little problematic still because it doesn't add any agent-specific command-line args. (--port would be a good one for demo.)

Right, good point. Since the example in the docs uses the long unused HWP Simulator Agent that doesn't add any arguments I thought I'd swap it out for a perhaps more familiar example, the FakeDataAgent. I've done that, and replaced instances of full file insertions with .. include:: directives so things stay up to date. That's done in 6979f6f.

The documentation in https://ocs.readthedocs.io/en/develop/developer/site_config.html#agent-site-related-command-line-parameters is pretty good, but it is not linked from the Developers:Agents section... it should be.

Ah, great, didn't realize that was there, it's now linked. Updated in 347d305.

Were there SOCS agents using it in a weird way?

I don't think so, it was just me trying to update it without knowing the full replacement, leaving behind an site_config.add_arguments() to make the initial parser, which caused the conflicts.

site_config.parse_args() seems suitable for agents but it is a little problematic for non-agents, such as clients and ocsbow... I think I have a fix for ocsbow at least; let me know if I should add it here or submit separately.

I'd say a fix for ocsbow could be separate.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

Sorry to sleep on this, if you were waiting for me. This is a draft PR but I guess it's ready to go? This looks good to me.

@BrianJKoopman BrianJKoopman marked this pull request as ready for review August 23, 2021 14:23
@BrianJKoopman
Copy link
Member Author

Sorry to sleep on this, if you were waiting for me. This is a draft PR but I guess it's ready to go? This looks good to me.

Thanks. I think I stalled out here when trying to write some tests for the behavior I thought I was trying to avoid. Going to go ahead with the merge now though after a rebase.

@BrianJKoopman BrianJKoopman merged commit eb05e11 into develop Aug 23, 2021
@BrianJKoopman BrianJKoopman deleted the argparse-updates branch August 23, 2021 14:33
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