Fix unit test and code check actions

[TayYim]

0. Description

This PR aims to fix the github actions of scenario_runner, which includes:

• Unit test
• Static Code Analysis
  • Check Code Format
  • Check Code Quality

In the previous several PRs, these actions failed.

This PR fixes Unit Test and Check Code Format.

There are tons of code changes, mainly contributed by autopep8.

If you want to check code changes only related to Unit Test, see commit

• 58656a3
• 1bd6d09
• 6aca6bf

1. Unit Test

I will show what issues I see and what changes I made here.

1.1 ConstantVelocityAgent

Commit: 6aca6bf

Unit Test failed because it can't import ConstantVelocityAgent.

Just copy the same file from Carla 0.9.15 to srunner/tests/carla_mocks/agents/navigation/constant_velocity_agent.py.

1.2 WeatherParameters

Commit: 1bd6d09

In srunner/scenarioconfigs/scenario_configuration.py L107:

self.weather = carla.WeatherParameters(sun_altitude_angle=70, cloudiness=50)

Here, the initialization parameters were passed to WeatherParameters. However, in srunner/tests/carla_mocks/carla.py, WeatherParameters did not have an init method. Therefore, I added an init method.

1.3 Infinite waiting for map

In scenario_runner/srunner/tests/test_xosc_load.py:

config = OpenScenarioConfiguration(filename, client, {})

Will wait forever. It turns out that there's a dead-loop in scenario_runner/srunner/scenarioconfigs/openscenario_configuration.py L209:

self.client.load_world(self.town)
while self.client.get_world().get_map().name.split('/')[-1] != self.town:
    continue

self.client.get_world().get_map().name gets nothing. So this loop never end.

The cause of this problem is that the World and Map classes do not define read and write operations for map_name in srunner/tests/carla_mocks/carla.py. Therefore, I have added the relevant setters and getters.

1.4 ActorConfigurationData init

Error msg:

File "scenario_runner/srunner/tools/openscenario_parser.py", line 681, in convert_position_to_transform
raise AttributeError("Object '{}' provided as position reference is not known".format(obj))
AttributeError: Object 'hero' provided as position reference is not known

It happens because in srunner/scenarioconfigs/openscenario_configuration.py, the ActorConfigurationData is initialized like

ActorConfigurationData(
            model=model, transform=None, rolename=rolename, speed=speed, color=color, category=category, args=args)

transform is None. I think it should be a default carla.Transform().

After I change None to carla.Transform(), issue has gone.

Warning

I'm not sure whether this modification is correct.

1.5 Empty actor list

srunner/tools/openscenario_parser.py L1281:

actor_transform = OpenScenarioParser.convert_position_to_transform(position)

It doesn't put an actor_list into the params. If it's a relative position, there will be no actor to be referenced.

Fixed:

actor_transform = OpenScenarioParser.convert_position_to_transform(position, config.other_actors + config.ego_vehicles)

2. Check Code Format

At local, I use this command to format python code in the whole project:

autopep8  --in-place --recursive . --max-line-length=120 --ignore=E731

I have also tried with the format commands in .github/workflows/static_code_check.yml:

autopep8 srunner/scenariomanager/*.py --in-place --max-line-length=120 --ignore=E731
autopep8 srunner/scenariomanager/scenarioatomics/*.py --in-place --max-line-length=120
autopep8 srunner/scenarios/*.py --in-place --max-line-length=120
autopep8 srunner/tools/*.py --in-place --max-line-length=120
autopep8 srunner/scenarioconfigs/*.py --in-place --max-line-length=120
autopep8 scenario_runner.py --in-place --max-line-length=120
autopep8 srunner/autoagents/*.py --in-place --max-line-length=120

However, even my code is well-formatted for sure, the Check Code Format actions fails.

Originally, the Check Code Format action check whether the code is met up to the pep8 standard by:

git diff --quiet HEAD --; if [ ! $? -eq 0 ]; then echo "Code is not autopep8 compliant. Please run code_check_and_formatting.sh"; git diff HEAD --; fi

This command works well locally, but always failed to execute in GitHub action.

Therefore, I decided to use a third-party action for autopep8 format checking directly: https://github.com/peter-evans/autopep8

The code in .github/workflows/static_code_check.yml is much simpler:

    - name: Check Code Format
      uses: peter-evans/[email protected]
      with:
        # Arguments to pass to autopep8
        args: --max-line-length=120 --ignore=E731

It works well then.

3. Check Code Quality

Well, this is hard.

Pylint has reported numerous points for optimization, including missing-function/module/class-docstring, unused-variable, attribute-defined-outside-init, unnecessary-lambda, among others.

I am unsure whether I should address each issue individually or adjust the Pylint configuration to suppress certain warnings.

---

This change is 

[Daraan]

1.4 ActorConfigurationData init

Error msg:

File "scenario_runner/srunner/tools/openscenario_parser.py", line 681, in convert_position_to_transform
raise AttributeError("Object '{}' provided as position reference is not known".format(obj))
AttributeError: Object 'hero' provided as position reference is not known

It happens because in srunner/scenarioconfigs/openscenario_configuration.py, the ActorConfigurationData is initialized like

ActorConfigurationData(
            model=model, transform=None, rolename=rolename, speed=speed, color=color, category=category, args=args)

transform is None. I think it should be a default carla.Transform().

After I change None to carla.Transform(), issue has gone.

Warning

I'm not sure whether this modification is correct.

---

I see a problem when setting it to a 0 Transform:

• _set_actor_information especially checks for transform None and replaces it with correct values.
• We might try to spawn actors at 0; see below.

Instead of throwing the AttributeError, I think we could return None. The propagation I can see are

• _get_actor_transform would handle the None case with returning a 0 Transform (so same result)
• CarlaDataProvider.request_new_actors would skip this actor if the transform is None; else it would try to spawn it at 0 which is something we do not want.

Maybe a conversion to a warning or raising at other locations might be better instead.

Question: Does 1.4 still persist if you revert 1.5?

Reviewable status: 0 of 56 files reviewed, 3 unresolved discussions (waiting on @TayYim)

---

srunner/scenarioconfigs/openscenario_configuration.py line 292 at r1 (raw file):

Setting a 0 Transform instead of None in the extract_*_information functions will skip this code section in the _set_actor_information

Code quote:

            for actor in self.other_actors + self.ego_vehicles:
                if actor.transform is None:
                    try:
                        actor.transform = self._get_actor_transform(actor.rolename)
                    except AttributeError as e:
                        if "Object '" in str(e):
                            ref_actor_rolename = str(e).split('\'')[1]
                            for ref_actor in self.other_actors + self.ego_vehicles:
                                if ref_actor.rolename == ref_actor_rolename:
                                    if ref_actor.transform is not None:
                                        raise e
                                    break
                        else:
                            raise e
                    if actor.transform is None:
                        all_actor_transforms_set = False

---

srunner/scenarioconfigs/openscenario_configuration.py line 310 at r1 (raw file):

        speed = self._get_actor_speed(rolename)
        new_actor = ActorConfigurationData(
            model, carla.Transform(), rolename, speed, color=color, category=category, args=args)

Should stay None or else skips transform retrieval in _set_actor_information

Code quote:

        new_actor = ActorConfigurationData(
            model, carla.Transform(), rolename, speed, color=color, category=category, args=args)

---

srunner/scenarioconfigs/openscenario_configuration.py line 324 at r1 (raw file):

        speed = self._get_actor_speed(rolename)
        new_actor = ActorConfigurationData(model, carla.Transform(), rolename, speed, category="pedestrian", args=args)

Should stay None or else skips transform retrieval in _set_actor_information

Code quote:

 new_actor = ActorConfigurationData(model, carla.Transform(),

---

srunner/scenarioconfigs/openscenario_configuration.py line 339 at r1 (raw file):

        else:
            model = misc.attrib.get('name')
        new_actor = ActorConfigurationData(model, carla.Transform(), rolename, category="misc", args=args)

Should stay None or else skips transform retrieval in _set_actor_information

Code quote:

  new_actor = ActorConfigurationData(model, carla.Transform(), 

[Daraan]

Reviewed 48 of 56 files at r1, all commit messages.
Reviewable status: 48 of 56 files reviewed, 11 unresolved discussions (waiting on @TayYim)

---

scenario_runner.py line 41 at r1 (raw file):

    def get_carla_version():
        """get the version of the CARLA package
        """

Single line would look nicer

Code quote:

        """get the version of the CARLA package
        """

---

scenario_runner.py line 49 at r1 (raw file):

    def get_carla_version():
        """same but for older Python versions
        """

Same as above; single line would look nicer.

---

srunner/metrics/examples/criteria_filter.py line 43 at r1 (raw file):

                            }
                            }
                           )

The old one wasn't perfect but I think the current layout is also not easy to follow.

Code quote:

            results.update({criterion_name:
                            {
                                "test_status": criterion["test_status"],
                                "actual_value": criterion["actual_value"],
                                "success_value": criterion["success_value"]
                            }
                            }
                           )

---

srunner/scenarioconfigs/osc2_scenario_configuration.py line 33 at r1 (raw file):

# pylint: enable=line-too-long

# OSC2

Comments do not align with their former lines

Code quote:

# pylint: enable=line-too-long

# OSC2

---

srunner/scenarios/open_scenario.py line 336 at r1 (raw file):

                                        atomic = ChangeActorWaypoints(carla_actor, waypoints=list(
                                            zip(waypoints, ['shortest'] * len(waypoints))),
                                            times=times, name="FollowTrajectoryAction")

The same hight of zip and and the next keyword argument looks confusing to me

Code quote:

                                            zip(waypoints, ['shortest'] * len(waypoints))),
                                            times=times, name="FollowTrajectoryAction")

---

srunner/tests/carla_mocks/carla.py line 181 at r1 (raw file):

                 wind_intensity=0.000000, sun_azimuth_angle=0.000000, sun_altitude_angle=0.000000,
                 fog_density=0.000000, fog_distance=0.000000, fog_falloff=0.000000, wetness=0.000000,
                 scattering_intensity=0.000000, mie_scattering_scale=0.000000, rayleigh_scattering_scale=0.033100):

The order:

 fog_density=0.000000, fog_distance=0.000000, fog_falloff=0.000000, wetness=0.000000,

is different from the carla docs

 fog_density=0.0, fog_distance=0.0, wetness=0.0, fog_falloff=0.0

Code quote:

    def __init__(self, cloudiness=0.000000, precipitation=0.000000, precipitation_deposits=0.000000,
                 wind_intensity=0.000000, sun_azimuth_angle=0.000000, sun_altitude_angle=0.000000,
                 fog_density=0.000000, fog_distance=0.000000, fog_falloff=0.000000, wetness=0.000000,
                 scattering_intensity=0.000000, mie_scattering_scale=0.000000, rayleigh_scattering_scale=0.033100):

---

tests/run_testcase/run_ast_testcases.py line 17 at r1 (raw file):

log_msg.is_open = True
# modify current working directory

Do we not need the statements down here after the sys.path adjustment?

Suggestion:

sys.path.append(os.getcwd())

# Moved import statements up

log_msg.is_open = True
# modify current working directory

---

tests/run_testcase/run_symbol_testcases.py line 16 at r1 (raw file):

log_msg.is_open = True

(Same as other) Do we not need the statements down here after the sys.path adjustment?