fix ila names

auto design were changed long ago and these functions did not
apply the changes. Besides there was a confusion between request_element
class where loose is a string, and PathRequest from topology.requests
where loose_list is a list of strings.
This patch corrects the naming and also the tests,
because it used the wrong class to gererate xls services

Signed-off-by: EstherLerouzic <[email protected]>
Change-Id: I564b77576459d6cb47767398a2db8138ba6ad1e4

gnpy/tools/convert.py

@@ -575,24 +575,41 @@ def corresp_names(input_filename, network):
                 corresp_ila[my_e.from_city] = [name]
     # complete with potential autodesign names: amplifiers
     for my_l in links:
-        name = f'Edfa0_fiber ({my_l.to_city} \u2192 {my_l.from_city})-{my_l.west_cable}'
-        if name in ila:
-            if my_l.from_city in corresp_ila.keys():
-                # "east edfa in Stbrieuc to Rennes_STA"  is equivalent name as
-                # "Edfa0_fiber (Lannion_CAS → Stbrieuc)-F056"
-                # "west edfa in Stbrieuc to Rennes_STA"  is equivalent name as
-                # "Edfa0_fiber (Rennes_STA → Stbrieuc)-F057"
-                # does not filter names: all types (except boosters) are created.
-                # in case fibers are splitted the name here is a prefix
-                corresp_ila[my_l.from_city].append(name)
-            else:
-                corresp_ila[my_l.from_city] = [name]
-        name = f'Edfa0_fiber ({my_l.from_city} \u2192 {my_l.to_city})-{my_l.east_cable}'
-        if name in ila:
-            if my_l.to_city in corresp_ila.keys():
-                corresp_ila[my_l.to_city].append(name)
-            else:
-                corresp_ila[my_l.to_city] = [name]
+        # create names whatever the type and filter them out
+        names = [f'Edfa_preamp_roadm {my_l.from_city}_from_fiber ({my_l.to_city} \u2192 {my_l.from_city})-{my_l.west_cable}',
+                 f'Edfa_booster_roadm {my_l.from_city}_to_fiber ({my_l.from_city} \u2192 {my_l.to_city})-{my_l.east_cable}']
+        for name in names:
+            if name in ila:
+                if my_l.from_city in corresp_ila.keys():
+                    # "east edfa in Stbrieuc to Rennes_STA"  is equivalent name as
+                    # "Edfa_booster_roadm Stbrieuc_to_fiber (Lannion_CAS → Stbrieuc)-F056"
+                    # "west edfa in Stbrieuc to Rennes_STA"  is equivalent name as
+                    # "Edfa_preamp_roadm Stbrieuc_to_fiber (Rennes_STA → Stbrieuc)-F057"
+                    # in case fibers are splitted the name here is a prefix
+                    corresp_ila[my_l.from_city].append(name)
+                else:
+                    corresp_ila[my_l.from_city] = [name]
+        names = [f'Edfa_preamp_roadm {my_l.to_city}_from_fiber ({my_l.from_city} \u2192 {my_l.to_city})-{my_l.east_cable}',
+                 f'Edfa_booster_roadm {my_l.to_city}_to_fiber ({my_l.to_city} \u2192 {my_l.from_city})-{my_l.west_cable}']
+        for name in names:
+            if name in ila:
+                if my_l.to_city in corresp_ila.keys():
+                    corresp_ila[my_l.to_city].append(name)
+                else:
+                    corresp_ila[my_l.to_city] = [name]
+
+    for node in nodes:
+        names = [f'east edfa in {node.city}', f'west edfa in {node.city}']
+        for name in names:
+            if name in ila:
+                if node.city in corresp_ila.keys():
+                    # "east edfa in Stbrieuc to Rennes_STA" (created with Eqpt) is equivalent name as
+                    # "east edfa in Stbrieuc" or "west edfa in Stbrieuc" (created with Links sheet)
+                    # depending on link node order
+                    corresp_ila[node.city].append(name)
+                else:
+                    corresp_ila[node.city] = [name]
+
     # merge fused with ila:
     for key, val in corresp_fused.items():
         if key in corresp_ila.keys():
@@ -781,16 +798,19 @@ def corresp_next_node(network, corresp_ila, corresp_roadm):
         user ILA name Stbrieuc covers the two direction. convert.py creates 2 different ILA
         with possible names (depending on the direction and if the eqpt was defined in eqpt
         sheet)
+        for an ILA and if it is defined in eqpt:
         - east edfa in Stbrieuc to Rennes_STA
         - west edfa in Stbrieuc to Rennes_STA
-        - Edfa0_fiber (Lannion_CAS → Stbrieuc)-F056
-        - Edfa0_fiber (Rennes_STA → Stbrieuc)-F057
+        for an ILA and if it is not defined in eqpt:
+        - east edfa in Stbrieuc
+        - west edfa in Stbrieuc
+        for a roadm
+        "Edfa_preamp_roadm node1_from_fiber (siteE → node1)-CABLES#19"
+        "Edfa_booster_roadm node1_to_fiber (node1 → siteE)-CABLES#19"
         next_nodes finds the user defined name of next node to be able to map the path constraints
         - east edfa in Stbrieuc to Rennes_STA      next node = Rennes_STA
         - west edfa in Stbrieuc to Rennes_STA      next node Lannion_CAS
 
-        Edfa0_fiber (Lannion_CAS → Stbrieuc)-F056 and Edfa0_fiber (Rennes_STA → Stbrieuc)-F057
-        do not exist
         the function supports fiber splitting, fused nodes and shall only be called if
         excel format is used for both network and service
     """
@@ -801,8 +821,8 @@ def corresp_next_node(network, corresp_ila, corresp_roadm):
         for ila_elem in ila_list:
             # find the node with ila_elem string _in_ the node uid. 'in' is used instead of
             # '==' to find composed nodes due to fiber splitting in autodesign.
-            # eg if elem_ila is 'Edfa0_fiber (Lannion_CAS → Stbrieuc)-F056',
-            # node uid 'Edfa0_fiber (Lannion_CAS → Stbrieuc)-F056_(1/2)' is possible
+            # eg if elem_ila is 'east edfa in Stbrieuc to Rennes_STA',
+            # node uid 'east edfa in Stbrieuc to Rennes_STA-_(1/2)' is possible
             correct_ila_name = next(n.uid for n in network.nodes() if ila_elem in n.uid)
             temp.remove(ila_elem)
             temp.append(correct_ila_name)

gnpy/tools/service_sheet.py

@@ -280,10 +280,8 @@ def correct_xls_route_list(network_filename, network, pathreqlist):
         # and last elem in the constraints respectively. Other positions must lead to an error
         # caught later on
         if pathreq.nodes_list and pathreq.source == pathreq.nodes_list[0]:
-            pathreq.loose_list.pop(0)
             pathreq.nodes_list.pop(0)
         if pathreq.nodes_list and pathreq.destination == pathreq.nodes_list[-1]:
-            pathreq.loose_list.pop(-1)
             pathreq.nodes_list.pop(-1)
         # Then process user defined constraints with respect to automatic namings
         temp = deepcopy(pathreq)
@@ -309,54 +307,50 @@ def correct_xls_route_list(network_filename, network, pathreqlist):
                         nodes_suggestion = corresp_ila[n_id]
                     else:
                         nodes_suggestion = []
-                if nodes_suggestion:
-                    try:
-                        if len(nodes_suggestion) > 1:
-                            # if there is more than one suggestion, we need to choose the direction
-                            # we rely on the next node provided by the user for this purpose
-                            new_n = next(n for n in nodes_suggestion
-                                         if n in next_node.keys() and next_node[n]
-                                         in temp.nodes_list[i:] + [pathreq.destination] and
-                                         next_node[n] not in temp.nodes_list[:i])
-                        else:
-                            new_n = nodes_suggestion[0]
-                        if new_n != n_id:
-                            # warns the user when the correct name is used only in verbose mode,
-                            # eg 'a' is a roadm and correct name is 'roadm a' or when there was
-                            # too much ambiguity, 'b' is an ila, its name can be:
-                            # Edfa0_fiber (a → b)-xx if next node is c or
-                            # Edfa0_fiber (c → b)-xx if next node is a
-                            msg = f'Request {pathreq.request_id}: Invalid route node specified:' \
-                                + f'\n\t\'{n_id}\', replaced with \'{new_n}\''
-                            logger.warning(msg)
-                            pathreq.nodes_list[pathreq.nodes_list.index(n_id)] = new_n
-                    except StopIteration:
-                        # shall not come in this case, unless requested direction does not exist
-                        msg = f'Request {pathreq.request_id}: Invalid route specified {n_id}: could' \
-                            + ' not decide on direction, skipped!.\nPlease add a valid' \
-                            + ' direction in constraints (next neighbour node)'
-                        logger.warning(msg)
-                        pathreq.loose_list.pop(pathreq.nodes_list.index(n_id))
-                        pathreq.nodes_list.remove(n_id)
-                else:
-                    if temp.loose_list[i] == 'LOOSE':
-                        # if no matching can be found in the network just ignore this constraint
-                        # if it is a loose constraint
-                        # warns the user that this node is not part of the topology
-                        msg = f'Request {pathreq.request_id}: Invalid node specified:\n\t\'{n_id}\'' \
-                            + ', could not use it as constraint, skipped!'
-                        logger.warning(msg)
-                        pathreq.loose_list.pop(pathreq.nodes_list.index(n_id))
-                        pathreq.nodes_list.remove(n_id)
+                try:
+                    if len(nodes_suggestion) > 1:
+                        # if there is more than one suggestion, we need to choose the direction
+                        # we rely on the next node provided by the user for this purpose
+                        new_n = next(n for n in nodes_suggestion
+                                        if n in next_node.keys() and next_node[n]
+                                        in temp.nodes_list[i:] + [pathreq.destination] and
+                                        next_node[n] not in temp.nodes_list[:i])
+                    elif len(nodes_suggestion) == 1:
+                        new_n = nodes_suggestion[0]
                     else:
-                        msg = f'Request {pathreq.request_id}: Could not find node:\n\t\'{n_id}\' in network' \
+                        if temp.loose == 'LOOSE':
+                            # if no matching can be found in the network just ignore this constraint
+                            # if it is a loose constraint
+                            # warns the user that this node is not part of the topology
+                            msg = f'{pathreq.request_id}: Invalid node specified:\n\t\'{n_id}\', ' \
+                                    + 'could not use it as constraint, skipped!'
+                            logger.warning(msg)
+                            pathreq.nodes_list.remove(n_id)
+                            continue
+                        msg = f'{pathreq.request_id}: Could not find node:\n\t\'{n_id}\' in network' \
                             + ' topology. Strict constraint can not be applied.'
+                        logger.critical(msg)
                         raise ServiceError(msg)
+                    if new_n != n_id:
+                        # warns the user when the correct name is used only in verbose mode,
+                        # eg 'a' is a roadm and correct name is 'roadm a' or when there was
+                        # too much ambiguity, 'b' is an ila, its name can be:
+                        # "east edfa in b to c", or "west edfa in b to a" if next node is c or
+                        # "west edfa in b to c", or "east edfa in b to a" if next node is a
+                        msg = f'Request {pathreq.request_id}: Invalid route node specified:' \
+                            + f'\n\t\'{n_id}\', replaced with \'{new_n}\''
+                        pathreq.nodes_list[pathreq.nodes_list.index(n_id)] = new_n
+                except StopIteration:
+                    # shall not come in this case, unless requested direction does not exist
+                    msg = f'Request {pathreq.request_id}: Invalid route specified {n_id}: could' \
+                        + ' not decide on direction, skipped!.\nPlease add a valid' \
+                        + ' direction in constraints (next neighbour node)'
+                    pathreq.nodes_list.remove(n_id)
             else:
-                if temp.loose_list[i] == 'LOOSE':
-                    logger.warning(f'Request {pathreq.request_id}: Invalid route node specified:\n\t\'{n_id}\''
-                                   + ' type is not supported as constraint with xls network input, skipped!')
-                    pathreq.loose_list.pop(pathreq.nodes_list.index(n_id))
+                if temp.loose == 'LOOSE':
+                    logger.warning('Invalid route node specified:\n\t\'%s\''
+                                   + ' type is not supported as constraint with xls network input,'
+                                   + ' skipped!', n_id)
                     pathreq.nodes_list.remove(n_id)
                 else:
                     msg = f'Invalid route node specified \n\t\'{n_id}\'' \

tests/test_parser.py

@@ -32,7 +32,7 @@
 from gnpy.tools.convert import convert_file
 from gnpy.tools.json_io import (load_json, load_network, save_network, load_equipment, requests_from_json,
                                 disjunctions_from_json, network_to_json, network_from_json)
-from gnpy.tools.service_sheet import read_service_sheet, correct_xls_route_list
+from gnpy.tools.service_sheet import read_service_sheet, correct_xls_route_list, Request_element, Request
 
 TEST_DIR = Path(__file__).parent
 DATA_DIR = TEST_DIR / 'data'
@@ -284,44 +284,43 @@ def test_json_response_generation(xls_input, expected_response_file):
 
 
 @pytest.mark.parametrize('source, destination, route_list, hoptype, expected_correction', [
-    ('trx Brest_KLA', 'trx Vannes_KBE',
+    ('Brest_KLA', 'Vannes_KBE',
         'roadm Brest_KLA | roadm Lannion_CAS | roadm Lorient_KMA | roadm Vannes_KBE',
-        'STRICT',
+        'no',
         ['roadm Brest_KLA', 'roadm Lannion_CAS', 'roadm Lorient_KMA', 'roadm Vannes_KBE']),
-    ('trx Brest_KLA', 'trx Vannes_KBE',
+    ('Brest_KLA', 'Vannes_KBE',
         'trx Brest_KLA | roadm Lannion_CAS | roadm Lorient_KMA | roadm Vannes_KBE',
-        'STRICT',
+        'No',
         ['roadm Lannion_CAS', 'roadm Lorient_KMA', 'roadm Vannes_KBE']),
-    ('trx Lannion_CAS', 'trx Rennes_STA', 'trx Rennes_STA', 'LOOSE', []),
-    ('trx Lannion_CAS', 'trx Lorient_KMA', 'toto', 'LOOSE', []),
-    ('trx Lannion_CAS', 'trx Lorient_KMA', 'toto', 'STRICT', 'Fail'),
-    ('trx Lannion_CAS', 'trx Lorient_KMA', 'Corlay | Loudeac | Lorient_KMA', 'LOOSE',
+    ('Lannion_CAS', 'Rennes_STA', 'trx Rennes_STA', 'yes', []),
+    ('Lannion_CAS', 'Lorient_KMA', 'toto', 'yes', []),
+    ('Lannion_CAS', 'Lorient_KMA', 'toto', 'no', 'Fail'),
+    ('Lannion_CAS', 'Lorient_KMA', 'Corlay | Loudeac | Lorient_KMA', 'yes',
         ['west fused spans in Corlay', 'west fused spans in Loudeac', 'roadm Lorient_KMA']),
-    ('trx Lannion_CAS', 'trx Lorient_KMA', 'Ploermel | Vannes_KBE', 'LOOSE',
+    ('Lannion_CAS', 'Lorient_KMA', 'Ploermel | Vannes_KBE', 'yes',
         ['east edfa in Ploermel to Vannes_KBE', 'roadm Vannes_KBE']),
-    ('trx Rennes_STA', 'trx Brest_KLA', 'Vannes_KBE | Quimper | Brest_KLA', 'LOOSE',
+    ('Rennes_STA', 'Brest_KLA', 'Vannes_KBE | Quimper | Brest_KLA', 'yes',
         ['roadm Vannes_KBE', 'west edfa in Quimper to Lorient_KMA', 'roadm Brest_KLA']),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | Quimper | Lorient_KMA', 'LOOSE',
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | Quimper | Lorient_KMA', 'yes',
         ['roadm Brest_KLA', 'east edfa in Quimper to Lorient_KMA', 'roadm Lorient_KMA']),
-    ('Brest_KLA', 'trx Rennes_STA', '', 'LOOSE', 'Fail'),
-    ('trx Brest_KLA', 'Rennes_STA', '', 'LOOSE', 'Fail'),
-    ('Brest_KLA', 'Rennes_STA', '', 'LOOSE', 'Fail'),
-    ('Brest_KLA', 'trx Rennes_STA', '', 'STRICT', 'Fail'),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'trx Rennes_STA', 'STRICT', []),
-    ('trx Brest_KLA', 'trx Rennes_STA', None, '', []),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | Quimper | Ploermel', 'LOOSE',
+    ('Brest_KLA', 'trx Rennes_STA', '', 'yes', 'Fail'),
+    ('trx Brest_KLA', 'Rennes_STA', '', 'yes', 'Fail'),
+    ('trx Brest_KLA', 'trx Rennes_STA', '', 'yes', 'Fail'),
+    ('Brest_KLA', 'trx Rennes_STA', '', 'no', 'Fail'),
+    ('Brest_KLA', 'Rennes_STA', 'trx Rennes_STA', 'no', []),
+    ('Brest_KLA', 'Rennes_STA', None, '', []),
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | Quimper | Ploermel', 'yes',
         ['roadm Brest_KLA']),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | Quimper | Ploermel', 'STRICT',
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | Quimper | Ploermel', 'no',
         ['roadm Brest_KLA']),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | trx Quimper', 'LOOSE', ['roadm Brest_KLA']),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | trx Lannion_CAS', 'LOOSE', ['roadm Brest_KLA']),
-    ('trx Brest_KLA', 'trx Rennes_STA', 'Brest_KLA | trx Lannion_CAS', 'STRICT', 'Fail')
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | trx Quimper', 'yes', ['roadm Brest_KLA']),
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | trx Lannion_CAS', 'yes', ['roadm Brest_KLA']),
+    ('Brest_KLA', 'Rennes_STA', 'Brest_KLA | trx Lannion_CAS', 'no', 'Fail')
 ])
 def test_excel_ila_constraints(source, destination, route_list, hoptype, expected_correction):
     """add different kind of constraints to test all correct_route cases"""
     service_xls_input = DATA_DIR / 'testTopology.xls'
     network_json_input = DATA_DIR / 'testTopology_auto_design_expected.json'
-    equipment = load_equipment(eqpt_filename)
     network = load_network(network_json_input, equipment)
     # increase length of one span to trigger automatic fiber splitting included by autodesign
     # so that the test also covers this case
@@ -331,37 +330,20 @@ def test_excel_ila_constraints(source, destination, route_list, hoptype, expecte
     p_db = default_si.power_dbm
     p_total_db = p_db + lin2db(automatic_nch(default_si.f_min, default_si.f_max, default_si.spacing))
     build_network(network, equipment, p_db, p_total_db)
-    # create params for a request based on input
-    nodes_list = route_list.split(' | ') if route_list is not None else []
+    # create params for a xls request based on input (note that this not the same type as PathRequest)
     params = {
         'request_id': '0',
         'source': source,
-        'bidir': False,
         'destination': destination,
-        'trx_type': '',
-        'trx_mode': '',
-        'format': '',
-        'spacing': '',
-        'nodes_list': nodes_list,
-        'loose_list': [hoptype for node in nodes_list] if route_list is not None else '',
-        'f_min': 0,
-        'f_max': 0,
-        'baud_rate': 0,
-        'OSNR': None,
-        'bit_rate': None,
-        'cost': None,
-        'roll_off': 0,
-        'tx_osnr': 0,
-        'tx_power': 0,
-        'penalties': None,
-        'min_spacing': None,
+        'trx_type': 'Voyager',
+        'spacing': 50,
+        'nodes_list': route_list,
+        'is_loose': hoptype,
         'nb_channel': 0,
         'power': 0,
         'path_bandwidth': 0,
-        'effective_freq_slot': None,
-        'equalization_offset_db': 0
     }
-    request = PathRequest(**params)
+    request = Request_element(Request(**params), equipment, False)
 
     if expected_correction != 'Fail':
         [request] = correct_xls_route_list(service_xls_input, network, [request])