Config and other defaults

[pv33]

Consider putting defaults in the core package folder-space such that it is automatically known and does not need to be provided by the calling script. In the case of surveillance, and assuming I understood the code, then `CfgNode should itself know where to get the default yaml file. If the executable must provide the filename, then doesn't seem to me that it is a default file.

The current implementation can be observed as being most likely improper because a change like this should be script-agnostic in the sense that major/critical changes to core running scripts should not be required. That's not the case here. Please correct.

Right now lots of stuff is breaking and its preventing me from advancing. Make sure to consider how updates affect other people's ability to collaboratively work with the code. I tried to communicate the changes through an issue when I made them and I provided example code to understand what was done. That's a simple courtesy to do always.

It's OK to include a multiple yaml file listing to artificially override the defaults with a new default (in the script). What the really means is that the "new" default will just override all of the fields of the core default file. It's inefficient, but should be something executed rarely, hence acceptable.

Also the config fields should probably align with the discussed class structure in a nominal manner. The core parts should probably be part of a set of Surveillance.Base fields, then new stuff in the Surveillance.Monitor fields, and then in a Surveillance.Manage field. The names don't have to fully agree. Like if they were "Surveillance", "Monitor", and "Manager" then it would be OK.

I am not sure how things have been organized? Where is there a description of the new yaml defaults and the files (if there are multiple such files). Where should I be looking for documentation? Consider creating a configs.md markdown file in Surveillance that describes the current state of things.

There's a few things in this issue. Make sure to cover them and provide response for each.

[pv33]

@yiyeChen confirming that default_yfile idea is bad. If I copy to my current scripts then I have to change the directory specification because the script is in another directory. If this is to be an internal file, then some other "instance" or workspace should be specifying and loading it so that the dafaults file location is agnostic to the running script's location. Doing otherwise is a great way to make fragile code. Please think things through a bit more thoroughly.

[pv33]

@yiyeChen config files also specify the source locations using a source field. That is now broken as far as I am aware. I should not have to specify the source files in the script proper as a "rosbag_name" hard coding. What gives? How do I get the kind of flexibility and minimal script massaging I created back?

Do you understand that the point of these yaml files is to avoid hard coding thigns into scripts that change from coder to coder, and that then cause unnecessary commits to occur that undo other people's code?

your exercise should be to strip all hard coded lines that adjust options within a script. For example, the current rosbag runner lines 594 to 600 should be deleted and things should still run. If that can't happen, then your changes are bad.

As a case in point config/testHandTracking01.yaml is not what I use. Rather I created my own version ``config/testHandTracking01pav.yaml``` that is unique to my OS setup and that is not pushed to the repository. It makes minor edits to the repo's version to work on my machine. All testing that I need to do has me using my slightly modified customized yaml files. That doesn't seem to be the case for other code.

[yiyeChen]

@pv33

Got it about the default loading. I will update the CfgNode class to automatically load the default parameters. Then the script can provide extra yaml files to overwrite the defaults or add new configuration.

I will update the namespace in the yaml files to align with the class structure. By the way can you attach the class structure graph to the trello? So that I can update the configuration namespace accordingly.

Right now all the Surveillance system parameters are in the config/default.yaml. It involves only the Surveillance system (segmenting the scene) configurations such as the camera setting, rostopic names, and the layer segmenter parameters. The configurations for the puzzle solver and the script usage (such as the rosbag_name, indicating what rosbag data to read) are not included. They stay as the way they were. The lines 594 to 600 (actually lines 575 - 602, including 3 options) you mentioned are Yunzhi's configurations for different test purpose. I didn't touch them because I didn't create and almost never use them. But if you need I can review and update them.

Although if I were to modify them, I am not sure whether we should put the configurations into different yaml files, or split into separate test scripts. I thought they should serve to inform how to use the puzzle solver for different purposes, in which case separate test scripts are more handy. Or maybe we do both.

[yiyeChen]

@pv33

Parameter passing mechanism is updated. The general principle is to make everything hierarchical as the system, including the configuration file, the parameters class, and the test script. The change is majorly made in the commit: e08a3feb4a734a3ddad027c83a9eae93a209e3e4.

The configuration file is hierarchical in the sense that parameters for different system components or usage can be separated. A parameter handling class for different purpose needs to be created to load the default parameters and add the pre-processings. The default loading and preprocessing are created in the load_defaults member function. By class inheritance, operations from the parent classes can be inherited while creating new functionality. In this way, the defaults can be loaded in the sequential manner following the hierarchical design. Noted that load_defaults must be run after the instance creation. In general, the configuration handling class should be written in the following manner:

class CfgNode_Parent(CfgNode_Base):

    def load_defaults(self):
        # Run the root load_defaults
        super().load_defaults()

        # Parent default loading
        default_file_Parent = PATH_TO_FILE
        self.merge_from_file(default_file_Parent)

        # Parent pre-processing
        self.example_parent = "ParentPreprocess"

# Child class inherits the parent class
class CfgNode_Child(CfgNode_Parent):

    def load_defaults(self, verbose=False):
        # Run the parent load_defaults
        super().load_defaults()

        # Child default loading
        default_file_Child = PATH_TO_FILE
        self.merge_from_file(default_file_Parent)

        # Child pre-processing
        self.example_child = "ChildPreprocess"

cfg = CfgNode_Child()
cfg.load_defaults() # Must run this. Will also load the parent parameters and execute parent preprocessings.

See the unit test in the utils/configs.py (after if __name__=="__main__") for example. The surveillance system core parameters are stored in the deployment/ROS/config/SystemDefaults.yaml, and some usage configurations are stored in the deployment/ROS/config/testHandTracking.yaml. In the utils/configs.py, the parent class CfgNode_Surv and the child class CfgNode_SurvRunner classes are created to load the core and usage defaults separately. Due to the mechanism explained above, the child CfgNode_SurvRunner instance alone can handle all the parameters (after load_defaults is called), which is used in the testing script deployment/ROS/Testing/rbtScene_new.

For the customized configurations, one needs to create a new yaml file to store the new settings, and use the merge_from_files function to update the parameters compared to the defaults in the test script. For example, I put my rosbag data path in the deployment/ROS/config/testHandTrackingYiye_eg.yaml. To apply the new setting I run:

./testing/rbtScene_new --yfile config/testHandTrackingYiye_eg.yaml

If want to use the default data path, just run:

./testing/rbtScene_new

[pv33]

@yiyeChen The original rbtScene did not require a flag --yfile and I liked that. Could you please modify the new rbtScene to have the same argument structure as the old one, then go ahead and overwrite it? Make sure that other files for which --yfile is the only available option also have it removed.

The whole purpose of git is to not need a duplicate file with modified functionality that also keeps the old one around. Please use git appropriately.

If I need anything old, I can look it up but going back to that commit either in the web server or through a checkout. Same should apply for any other developer of this repo.

[yiyeChen]

@pv33

Got it. Will remove the old.

One problem with removing -- is that this is the symbol for marking the optional arguments as opposed to the mendatory ones. If we use yfile w/o -- then we must provide something to the script, even if we want to use the default parameters.

But of course we can pass something meaningless, and let the code decide. This means that if we want to use the default parameters, we will need to run something like this:

./rbtScene.py None

rather than just

./rbtScene.py

Can you confirm this is acceptable? If that is true then I can change to that way.

[pv33]

@yiyeChen

First

Make sure that other files for which --yfile is the only available option also have it removed.

Holds for case of single optional argument for sure due to this setup working in original rbtScene.

Second,

That can't be true because I was using rbtScene with and without the optional argument and it seemed to be working. In the absence of the specified file, it would load some default file. It would automatically fill in with a None if there was no file provided. According to argparse documentation my implementation was correct. A non-optional argument is usually assumed to be a final positional argument (though most likely it can also go first if argument parsing conforms to linux rules).

Yes it is acceptable. Yes it is possible to have a non dashed optional argument with specified positional form relative to the optional ones, and it is possible in python to properly parse things.

On a related note, I am not sure how you are dealing with the runtime configuration. There should only be one default file if it is going to be internally loaded and it should not be prefaced with the name "test" plus it should apply to the entire system. That doesn't seem to be the case since the only non-"test" yaml file is SystemDefaults when there should be another one maybe called RuntimeDefaults that gets overloaded as needed.

Not sure what you did nor what you were thinking.

[pv33]

@yiyeChen

Confirming that positional arguments can go anywhere as per argparse documentation. However, they do have an ordering when given, whereas optional ones have no ordering by virtue of being directly specified by the -- flags.

[pv33]

Not sure why it worked for me, but argparse requires a special flag for a possibly missing positional argument:

see docs here

A default can be given.

[yiyeChen]

@pv33

Okay I see how to do it. For the present of a positional argument to be optional, it is not enough to just provide with a default value, but also requires to set nargs="?". No I have updated all the related files accordingly.

Oh yes about the configuration file. The name is confusing. I will change the default one to RuntimeDefaults.yaml