CGMES : Implement a pre-processor that completes the CGMES model with missing containers before starting the conversion #2463

zamarrenolm · 2023-02-02T17:54:58Z

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
#2458

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
CGMES import fails if containers (voltage levels) are referred but not defined in the set of input XML files.

What is the new behavior (if this is a feature change)?
A CGMES pre-processor scheme is proposed, following a similar approach to current post-processors.
Here we want to add plugins that intervene in the CGMES model AFTER the given CGMES data has been read but BEFORE the conversion is started.
This provides a point where we can configure a potentially time consuming process. It will be called only if the user activates it.
We inspect the initial CGMES model looking for missing data and try to complete it before the conversion starts.

Other information:
The implementation will build a new XML file that will contain the missing definitions.
After reading it to add its contents to the CGMES model in memory, we have to invalidate all previous caches.

… missing containers before starting the conversion Signed-off-by: Luma <zamarrenolm@aia.es>

miovd

Some remarks to open a discussion

...nversion/src/main/java/com/powsybl/cgmes/conversion/DefineMissingContainersPreProcessor.java

cgmes/cgmes-model/src/main/resources/CIM16.sparql

Signed-off-by: Luma <zamarrenolm@aia.es>

…itten Signed-off-by: Luma <zamarrenolm@aia.es>

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm · 2023-03-14T11:23:48Z

When no folder is specified as parameter, the completion files are written in a temp directory created from the current user.dir to avoid the security risk defined in https://rules.sonarsource.com/java/RSPEC-5443.

miovd

I'm wondering if this pre-processor's place is in core at all.... The feature of CGMES pre-processors in itself is very nice and a good idea! But this one is, I think, about a use-case that is too specific to be in core. Maybe it can be in another repo? @annetill do you have any opinion?

miovd · 2023-03-14T12:43:22Z

cgmes/cgmes-completion/pom.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Copyright (c) 2017, RTE (http://www.rte-france.com)


miovd · 2023-03-14T12:44:12Z

cgmes/cgmes-completion/src/test/resources/logback-test.xml

+    <logger name="com.powsybl.triplestore.TripleStore" level="error"/>
+    <logger name="com.powsybl.cgmes.triplestore.CgmesModelTripleStore" level="error"/>
+    <logger name="com.powsybl.cgmes.conversion.Conversion" level="error"/>
+    <logger name="com.powsybl.cgmes.completion" level="info"/>


Why is the log level info here?

For the unit tests, to have easy access to the relevant info about the run (location of the output file, list of missing voltage levels in the input) ...

It can be modified by the dev for debug, but I think by default, it should be error to limit logs during simple compilation

log level updated to error

miovd · 2023-03-14T12:45:05Z

...gmes-conversion/src/test/java/com/powsybl/cgmes/conversion/CgmesImportPostProcessorTest.java

@@ -28,6 +29,7 @@
 /**
 * @author Geoffroy Jamgotchian <geoffroy.jamgotchian at rte-france.com>
 */
+// FIXME(Luma) rename to CgmesImportProcessorsTest (we will test both pre- and post- processors)


You can rename it now :)

Yes, I forgot to do it. I have renamed the test class as CgmesImportPreAndPostProcessorsTest

miovd · 2023-03-14T12:45:53Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/DefineMissingContainersPreProcessor.java

+ * @author Luma Zamarreño <zamarrenolm at aia.es>
+ */
+@AutoService(CgmesImportPreProcessor.class)
+public class DefineMissingContainersPreProcessor implements CgmesImportPreProcessor {


Maybe CreateMissingContainersPreProcessor is more explicit?

miovd · 2023-03-14T12:50:05Z

I'm wondering if this pre-processor's place is in core at all.... The feature of CGMES pre-processors in itself is very nice and a good idea! But this one is, I think, about a use-case that is too specific to be in core. Maybe it can be in another repo? @annetill do you have any opinion?

To explain my viewpoint more in details, there are some processors we use in private repo because they are too specific to one company's use cases and I think this is the same here... Core is supposed to have standards processes. Even ENTSO-E processes are not in here.

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm · 2023-03-21T17:37:27Z

Generic scheme for pre-processors has been moved to #2513. Once it is merged in main, we will leave here only the specific code for completing CGMES model with missing containers using the pre-processor mechanism.

Signed-off-by: Luma <zamarrenolm@aia.es>

sonarqubecloud · 2023-03-31T05:59:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

85.2% Coverage
0.0% Duplication

miovd · 2023-05-25T08:13:04Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+@AutoService(CgmesImportPreProcessor.class)
+public class CreateMissingContainersPreProcessor implements CgmesImportPreProcessor {
+
+    public static final String NAME = "CreateMissingContainers";


For homogenisation purposes, you can simply name it missingContainers

But for example the IIDM modification post-processor that replaces tie lines by lines starts also with a verb (replaceTieLinesByLines). I prefer if we keep the action in the name. What do you think?

I think there is a difference between modifications (which state the action they do) and processors (which state the context, particularly for CGMES processors) but both are okay in my opinion but it should start with a low case

The name now starts with lower case, createMissingContainers.
I think of pre/post-processors as specialized tools carrying out specific tasks before/after CGMES import, so for me it is ok if their name starts with a verb.

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

miovd · 2023-05-25T08:14:48Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+
+    private static void buildZipFileWithFixes(Set<String> missingVoltageLevels, Path fixesFile, String basename) {
+        Network network = NetworkFactory.findDefault().createNetwork("empty", "CGMES");
+        // TODO(Luma) should we ensure that context CIM version uses the same version of the input files ?


Can you elaborate this TODO for me? Not sure I understand

We have read CGMES files in a given version.
We should export CGMES files in the same version.
When the network is created applying conversion from CGMES files, the version of input data is stored in the extension CimCharacteristics, and when an export context is created from a Network, the version to use in the export is the same one that was read, unless explicitly changed.
Here we are creating an export context with an empty Network that will not carry information about specific CIM version to use, so the default value will be used (CIM16).

I will set the CIM version in the context explicitly, based on the version of the input files.

Oh okay, I think you directly retrieve it from the model:

int cim = 16; if (model instanceof CgmesModelTriplestore) { cim = ((CgmesModelTriplestore) model).getCimVersion(); } else { // warning }

Yes, it is what is done in the method prepareEmptyNetworkForExport. The CIM version read from the input files is set in the CimCharacteristics extension of the "empty" IIDM Network that is used to generate the CGMES files that will define the missing containers.

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

miovd · 2023-05-25T08:23:06Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+            LOG.warn("Missing fixes folder parameter {}, attempt to create a temp directory in user working directory.", FIXES_FOLDER_NAME_PARAMETER.getName());
+            Path current = Path.of(System.getProperty("user.dir"));
+            String prefix = "cgmes-fixes-for-missing-containers";
+            boolean debug = false;
+            try (WorkingDirectory workingDirectory = new WorkingDirectory(current, prefix, debug)) {
+                buildFixesOnFolder(cgmes, basename, workingDirectory.toPath());


Not sure I agree with this.... Not really a good thing to "silently" write. For me, it should like usual export, if not defined, it should either fail or at the very least, use directory of PlatformConfig but I think we wanted to limit the use of that (@flo-dup you can correct me if I'm wrong)

When not specified through the corresponding parameter the pre-processor does not run. An error is logged.
For the tests I have used a Jimfs filesystem, through the test platform config and local computation manager.

Okay, after re-reading, I think you need to add a FileSystem as parameter in the preprocessor (by default FileSystem.getDefault()) and use that to create/delete files. It is cleaner and that would also allow to test it properly.

I was able to solve the issue (and do proper testing) without the need of a explicit additional parameter for FileSystem.

When we obtain the folder parameter from config:

if it is an absolute path the default file system is used.

If the path is relative, it is assumed relative to the platform config dir.

In the tests, we create a platform config that lives in the Jimfs file system, following the same schema applied in other unit tests.

Yes, I left a comment below, your solution is okay for me

miovd · 2023-05-25T08:24:07Z

cgmes/cgmes-completion/src/test/resources/logback-test.xml

+    <logger name="com.powsybl.triplestore.TripleStore" level="error"/>
+    <logger name="com.powsybl.cgmes.triplestore.CgmesModelTripleStore" level="error"/>
+    <logger name="com.powsybl.cgmes.conversion.Conversion" level="error"/>
+    <logger name="com.powsybl.cgmes.completion" level="info"/>


It can be modified by the dev for debug, but I think by default, it should be error to limit logs during simple compilation

miovd · 2023-05-25T08:25:09Z

cgmes/cgmes-completion/src/test/java/com/powsybl/cgmes/completion/CgmesCompletionTest.java

+        // The only way to pass the output folder where we want the fixes to be written is to use a config file
+        // Its contents should be:
+        //
+        //   import-export-parameters-default-value:
+        //     iidm.import.cgmes.fixes-for-missing-containers-filename: "/user/working/area/fixes/..."
+        //
+        // For tests, it is not possible to put a reference to a Jimfs folder,
+        // Even if we write the uri of the folder, and read it back as an uri,
+        // the default file system is used to interpret it
+        //
+        // An alternative would be to write in the config a temp folder that we create here in the tests,
+        // But that means writing in a resource file (the actual path of the temp folder), it is dirty.
+        //
+        // Instead, the CGMES completion processor creates itself a temp folder when no parameter is received.
+        // We won't receive directly the file with the fixes, but we do not care.


I don't see the issue with writing a temporary file using Jimfs URIs, it is cleaner that writing temporarily in user.dir in my opinion

I did not want to use Jimfs directly in the preprocessor: in the rest of PowSyBl core this library is only included in test code. It will add a new dependency in the framework.

The problem is that we can not setup a Jimfs folder inside the test code that is then used in the preprocessor: no way to put a reference in the config file.

Solved the issue passing a computation manager with a test platform config.
During the tests the temp files are written to a Jimfs filesystem.

Just seen this message, I'm keeping the previous one, not sure which solution is best, a suggestion @olperr1 ? Both are okay in my opinion

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm · 2023-06-13T10:41:47Z

Removed the "silent" write if the output folder for fixes is not defined in the configuration.
Adjusted the test to use Jimfs in this scenario.
When an output folder for the files containing the fixes is defined in the configuration: if it is an absolute path it is taken as it is. If it is relative, it is assumed to be relative the the config dir. This allows to create a test config on Jimfs and the pre-processor to use it.

miovd · 2023-06-15T07:42:10Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+import java.util.zip.ZipOutputStream;
+
+/**
+ * @author Luma Zamarreño <zamarrenolm at aia.es>


Maybe a (light) javadoc, just to explain what is done in this pre-processor?

added a small javadoc

miovd · 2023-06-15T07:47:39Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+@AutoService(CgmesImportPreProcessor.class)
+public class CreateMissingContainersPreProcessor implements CgmesImportPreProcessor {
+
+    public static final String NAME = "CreateMissingContainers";


I think there is a difference between modifications (which state the action they do) and processors (which state the context, particularly for CGMES processors) but both are okay in my opinion but it should start with a low case

miovd · 2023-06-15T07:56:08Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+
+    private static void buildZipFileWithFixes(Set<String> missingVoltageLevels, Path fixesFile, String basename) {
+        Network network = NetworkFactory.findDefault().createNetwork("empty", "CGMES");
+        // TODO(Luma) should we ensure that context CIM version uses the same version of the input files ?


Oh okay, I think you directly retrieve it from the model:

int cim = 16; if (model instanceof CgmesModelTriplestore) { cim = ((CgmesModelTriplestore) model).getCimVersion(); } else { // warning }

miovd · 2023-06-15T07:58:18Z

...mpletion/src/main/java/com/powsybl/cgmes/completion/CreateMissingContainersPreProcessor.java

+            LOG.warn("Missing fixes folder parameter {}, attempt to create a temp directory in user working directory.", FIXES_FOLDER_NAME_PARAMETER.getName());
+            Path current = Path.of(System.getProperty("user.dir"));
+            String prefix = "cgmes-fixes-for-missing-containers";
+            boolean debug = false;
+            try (WorkingDirectory workingDirectory = new WorkingDirectory(current, prefix, debug)) {
+                buildFixesOnFolder(cgmes, basename, workingDirectory.toPath());


Okay, after re-reading, I think you need to add a FileSystem as parameter in the preprocessor (by default FileSystem.getDefault()) and use that to create/delete files. It is cleaner and that would also allow to test it properly.

miovd · 2023-06-15T07:59:37Z

cgmes/cgmes-completion/src/test/java/com/powsybl/cgmes/completion/CgmesCompletionTest.java

+        // The only way to pass the output folder where we want the fixes to be written is to use a config file
+        // Its contents should be:
+        //
+        //   import-export-parameters-default-value:
+        //     iidm.import.cgmes.fixes-for-missing-containers-filename: "/user/working/area/fixes/..."
+        //
+        // For tests, it is not possible to put a reference to a Jimfs folder,
+        // Even if we write the uri of the folder, and read it back as an uri,
+        // the default file system is used to interpret it
+        //
+        // An alternative would be to write in the config a temp folder that we create here in the tests,
+        // But that means writing in a resource file (the actual path of the temp folder), it is dirty.
+        //
+        // Instead, the CGMES completion processor creates itself a temp folder when no parameter is received.
+        // We won't receive directly the file with the fixes, but we do not care.


Just seen this message, I'm keeping the previous one, not sure which solution is best, a suggestion @olperr1 ? Both are okay in my opinion

Signed-off-by: Luma <zamarrenolm@aia.es>

sonarqubecloud · 2023-06-15T09:17:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

81.9% Coverage
0.0% Duplication

CGMES : Implement a pre-processor that completes the CGMES model with…

c187c89

… missing containers before starting the conversion Signed-off-by: Luma <zamarrenolm@aia.es>

miovd reviewed Feb 6, 2023

View reviewed changes

...nversion/src/main/java/com/powsybl/cgmes/conversion/DefineMissingContainersPreProcessor.java Outdated Show resolved Hide resolved

cgmes/cgmes-model/src/main/resources/CIM16.sparql Outdated Show resolved Hide resolved

zamarrenolm added 15 commits February 7, 2023 16:20

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

6bd2386

separate module for CGMES completion post-processors

8c99a09

Signed-off-by: Luma <zamarrenolm@aia.es>

update comment

d503a19

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

00ec877

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

1685aeb

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

d0946f5

accept parameter for output folder, where files with fixes will be wr…

3e9da15

…itten Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

98ce590

write EQ instance file with missing data

6827a2a

Signed-off-by: Luma <zamarrenolm@aia.es>

comment about fictitious substation from equipment export

11f2bb4

Signed-off-by: Luma <zamarrenolm@aia.es>

fix comment

378ff5c

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

9c9ce21

Signed-off-by: Luma <zamarrenolm@aia.es>

create temp folder in user.dir to fix Sonar security risk java:S5443

0f938b0

Signed-off-by: Luma <zamarrenolm@aia.es>

fix sonar code smell

40862eb

Signed-off-by: Luma <zamarrenolm@aia.es>

add cgmes completion module to distribution

248b56c

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm marked this pull request as ready for review March 14, 2023 11:18

zamarrenolm requested a review from miovd March 14, 2023 11:18

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

71c41ce

miovd reviewed Mar 14, 2023

View reviewed changes

zamarrenolm added 5 commits March 14, 2023 14:52

changes after review

02af044

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

049104f

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

349b00e

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

870bba6

update pom.xml

80e1484

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm marked this pull request as draft March 21, 2023 17:37

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

cd2c56f

Signed-off-by: Luma <zamarrenolm@aia.es>

zamarrenolm marked this pull request as ready for review March 31, 2023 07:03

zamarrenolm requested a review from miovd March 31, 2023 07:03

annetill added the PR: next-release label May 25, 2023

miovd requested changes May 25, 2023

View reviewed changes

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

8689908

olperr1 removed the PR: next-release label Jun 5, 2023

zamarrenolm added 4 commits June 12, 2023 15:21

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

d0f3ff3

changes after review

f602a1b

Signed-off-by: Luma <zamarrenolm@aia.es>

test output temporal files to Jimfs

e3bcf96

Signed-off-by: Luma <zamarrenolm@aia.es>

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

ee8b339

zamarrenolm requested a review from miovd June 13, 2023 10:41

Merge branch 'main' into cgmes_pre_processor_define_missing_containers

0c0dac6

miovd reviewed Jun 15, 2023

View reviewed changes

add javadoc

927908e

Signed-off-by: Luma <zamarrenolm@aia.es>

miovd approved these changes Jun 15, 2023

View reviewed changes

annetill added the PR: ready-to-be-merged label Jun 15, 2023

annetill merged commit 7658e1a into main Jun 15, 2023

annetill deleted the cgmes_pre_processor_define_missing_containers branch June 15, 2023 09:34

CGMES : Implement a pre-processor that completes the CGMES model with missing containers before starting the conversion #2463

CGMES : Implement a pre-processor that completes the CGMES model with missing containers before starting the conversion #2463

Conversation

zamarrenolm commented Feb 2, 2023

miovd left a comment

Choose a reason for hiding this comment

zamarrenolm commented Mar 14, 2023

miovd 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

miovd commented Mar 14, 2023

zamarrenolm commented Mar 21, 2023

sonarqubecloud bot commented Mar 31, 2023

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

miovd May 25, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zamarrenolm commented Jun 13, 2023

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

sonarqubecloud bot commented Jun 15, 2023

miovd May 25, 2023 •

edited

Loading