Generator refactoring #1410

5A11 · 2020-06-23T16:03:52Z

Proposed changes

This PR breaks the code in the generator module into the following modules:

validate: containing code used to validate a protocol specification
extract_specification: containing code to extract fields from a protocol specification into pythonic objects (all fields are encapsulated in PythonicProtocolSpecification)
common: containing constants and functions used in more than one module
generator: containing the old generator code minus the above separated codes

The PR doesn't do anything else other than breaking existing functionality into separate modules (except some minor necessary tweaks to make that happen).

Some notes:

validate is now supposed to just take a protocol specification and indicates whether it is a valid specification or not. Crucially it no longer makes any conversion to Python code/objects. This means it can be reused before conversions of specification to other languages. Also, other format checking logic would later go into this module.

extract_specification is placed into its own module mainly because the extraction logic has many dependencies (private functions, constants, etc) which cluttered up the old generator and they could logically be grouped together in an independent way. extract() automatically calls validate() because it always needs a valid specification to do its work.

All the above are now placed in a generator package under aea.protocols

Fixes #976

Types of changes

What types of changes does your code introduce to agents-aea?
Put an x in the boxes that apply

Bugfix (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

Put an x in the boxes that apply.

I have read the CONTRIBUTING doc
I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have checked that code coverage does not decrease.
I have checked that the documentation about the aea cli tool works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in downstream modules

5A11 · 2020-06-23T16:05:47Z

aea/protocols/generator/common.py

@@ -0,0 +1,110 @@
+# -*- coding: utf-8 -*-


This module has code common to all generator modules. Currently only one function and one constant.

5A11 · 2020-06-23T16:09:27Z

aea/protocols/generator/extract_specification.py

@@ -0,0 +1,262 @@
+# -*- coding: utf-8 -*-


This module now has the logic to extract protocol specification fields into pythonic equivalences.

It has one function extract() with all its private sub-functions. It extracts all specification fields into python objects and encapsulates them into a PythonicProtocolSpecification class.

I separated these code into its own module mainly to declutter the main generator and because they have a nice logical grouping.

5A11 · 2020-06-23T16:10:51Z

aea/protocols/generator/validate.py

@@ -0,0 +1,101 @@
+# -*- coding: utf-8 -*-


This module has code to validate a protocol specification. It crucially no longer does validation on Pythonic conversion of specification fields. All validation is done purely on the specification.

This means it can be reused just before converting specification to other languages.

It has a single validate() function (plus private validate sub-functions) that gets a protocol specification and indicates whether it is valid or not. I did not put validate() into a class because I failed to see a reason for it and I think it would just complicate the code.

Currently it has the existing validation logic. Other format checking logic (an outstanding generator issue) will be added in this module.

codecov-commenter · 2020-06-23T16:15:30Z

Codecov Report

Merging #1410 into develop will increase coverage by 0.62%.
The diff coverage is 92.20%.

@@             Coverage Diff             @@
##           develop    #1410      +/-   ##
===========================================
+ Coverage    90.10%   90.73%   +0.62%     
===========================================
  Files          213      217       +4     
  Lines        15205    15242      +37     
===========================================
+ Hits         13701    13830     +129     
+ Misses        1504     1412      -92

Flag	Coverage Δ
#unittests	`90.73% <92.20%> (+0.62%)`	⬆️

Impacted Files	Coverage Δ
aea/protocols/generator/validate.py	`74.28% <74.28%> (ø)`
aea/protocols/generator/base.py	`83.94% <86.27%> (ø)`
aea/protocols/generator/common.py	`97.50% <97.50%> (ø)`
aea/protocols/generator/extract_specification.py	`99.02% <99.02%> (ø)`
aea/cli/generate.py	`100.00% <100.00%> (ø)`
aea/protocols/generator/__init__.py	`100.00% <100.00%> (ø)`
aea/skills/base.py	`97.05% <0.00%> (-0.25%)`	⬇️
aea/aea_builder.py	`86.27% <0.00%> (-0.14%)`	⬇️
aea/helpers/exec_timeout.py	`100.00% <0.00%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f12e82...65cc583. Read the comment docs.

DavidMinarsch

Looks good so far, some comments

DavidMinarsch · 2020-06-23T16:10:23Z

aea/protocols/generator/generator.py

        """
-        Create the protocol package with Message, Serialization, __init__, protocol.yaml files.
+        Run the generator. If in "full" mode (protobuf_only is False), it:
+        a) validates the protocol specification.


Validation seems to be happening elsewhere, I cannot see it below.

Validation is a prerequisite for extraction so extract() already calls validate().

The docstring gives an overall explanation of major things that happen in the two modes.

DavidMinarsch · 2020-06-23T16:12:38Z

aea/protocols/generator/generator.py

+        if not protobuf_only:
+            # Generate Python files
+            self._generate_file(INIT_FILE_NAME, self._init_str())
+            self._generate_file(PROTOCOL_YAML_FILE_NAME, self._protocol_yaml_str())
+            self._generate_file(MESSAGE_DOT_PY_FILE_NAME, self._message_class_str())
+            if (
+                self.protocol_specification.dialogue_config is not None
+                and self.protocol_specification.dialogue_config != {}
+            ):
+                self._generate_file(
+                    DIALOGUE_DOT_PY_FILE_NAME, self._dialogue_class_str()
+                )
+            if len(self.spec.all_custom_types) > 0:
+                self._generate_file(
+                    CUSTOM_TYPES_DOT_PY_FILE_NAME, self._custom_types_module_str()
+                )
+            self._generate_file(
+                SERIALIZATION_DOT_PY_FILE_NAME, self._serialization_class_str()
            )


This should be wrapped in a public method generate_python_files so it becomes a single line here

DavidMinarsch · 2020-06-23T16:13:48Z

aea/protocols/generator/generator.py


-        # Warn if specification has custom types
-        if len(self._all_custom_types) > 0:
-            incomplete_generation_warning_msg = "The generated protocol is incomplete, because the protocol specification contains the following custom types: {}. Update the generated '{}' file with the appropriate implementations of these custom types.".format(
-                self._all_custom_types, CUSTOM_TYPES_DOT_PY_FILE_NAME
+        if not protobuf_only:


Have a guard clause here:

if protobuf_only: return

DavidMinarsch · 2020-06-23T16:15:47Z

aea/protocols/generator/generator.py

@@ -26,12 +26,11 @@
 from datetime import date


Better to call it base.py to align with our naming conventions.

DavidMinarsch · 2020-06-23T16:17:46Z

aea/cli/generate.py

@@ -38,7 +38,7 @@
    PublicId,
 )
 from aea.configurations.loader import ConfigLoader
-from aea.protocols.generator import ProtocolGenerator
+from aea.protocols.generator.generator import ProtocolGenerator


Your init file below is missing header info

DavidMinarsch

LGTM

breaking up the generator module into various modules

681a756

5A11 requested a review from DavidMinarsch June 23, 2020 16:03

5A11 commented Jun 23, 2020

View reviewed changes

DavidMinarsch reviewed Jun 23, 2020

View reviewed changes

5A11 added 2 commits June 24, 2020 09:24

minor changes to address pr comments

eb7f4aa

formatting

65cc583

DavidMinarsch approved these changes Jun 24, 2020

View reviewed changes

DavidMinarsch merged commit 394d491 into develop Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generator refactoring #1410

Generator refactoring #1410

5A11 commented Jun 23, 2020 •

edited by DavidMinarsch

Loading

5A11 Jun 23, 2020

5A11 Jun 23, 2020

5A11 Jun 23, 2020 •

edited

Loading

5A11 Jun 23, 2020

codecov-commenter commented Jun 23, 2020 •

edited

Loading

DavidMinarsch left a comment

DavidMinarsch Jun 23, 2020

5A11 Jun 23, 2020

DavidMinarsch Jun 23, 2020

5A11 Jun 24, 2020

DavidMinarsch Jun 23, 2020

5A11 Jun 24, 2020

DavidMinarsch Jun 23, 2020

5A11 Jun 24, 2020

DavidMinarsch Jun 23, 2020

5A11 Jun 24, 2020

DavidMinarsch left a comment

Generator refactoring #1410

Generator refactoring #1410

Conversation

5A11 commented Jun 23, 2020 • edited by DavidMinarsch Loading

Proposed changes

Types of changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5A11 Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 23, 2020 • edited Loading

Codecov Report

DavidMinarsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidMinarsch left a comment

Choose a reason for hiding this comment

5A11 commented Jun 23, 2020 •

edited by DavidMinarsch

Loading

5A11 Jun 23, 2020 •

edited

Loading

codecov-commenter commented Jun 23, 2020 •

edited

Loading