Simplify xmi to liquid #136

kwkwan · 2025-02-27T00:59:15Z

No description provided.

hound · 2025-02-27T00:59:35Z

spec/express/parsers/exp_spec.rb

@@ -16,7 +16,7 @@
      end

      it "correctly reads schema attributes" do
-        expect(parse.schemas.first.id).to(eq('annotated_3d_model_data_quality_criteria_schema'))
+        expect(parse.schemas.first.id).to(eq("annotated_3d_model_data_quality_criteria_schema"))


Line is too long. [96/80]

hound · 2025-02-27T00:59:35Z

lib/lutaml/xmi/parsers/xmi_base.rb

+        # @param link [Lutaml::Model::Serializable]
+        # @param link_member_name [String]
+        # @return [Array<String, String, Hash, String, String>]
+        def serialize_member_type(owner_xmi_id, link, link_member_name)


Method has too many lines. [11/10]

hound · 2025-02-27T00:59:35Z

lib/lutaml/xmi/parsers/xmi_base.rb

+
+        # @param general_id [String]
+        # @return [Array<Hash>]
+        def get_general_hash(general_id)


Method has too many lines. [15/10]

hound · 2025-02-27T00:59:36Z

lib/lutaml/sysml/block.rb

-		return nil
-	end
-end
+      def initialize


Call super to initialize state of the parent class.

hound · 2025-02-27T00:59:36Z

lib/lutaml/sysml/constraint_block.rb

+module Lutaml
+  module SysMl
+    class ConstraintBlock < Block
+      def initialize


Call super to initialize state of the parent class.

hound · 2025-02-27T00:59:36Z

lib/lutaml/sysml/nested_connector_end.rb

-	def initialize
-		@property_path = []
-	end
+      def initialize


Call super to initialize state of the parent class.

hound · 2025-02-27T00:59:36Z

lib/lutaml/sysml/xmi_file.rb

+        end
+
+        if (["uml:Property", "uml:Port"].include?(xml_node.attribute_with_ns("type",
+                                      xmiNS.href).to_s)) && not ["definingFeature", "partWithPort", "propertyPath",


unexpected token tLBRACK

hound · 2025-02-27T00:59:36Z

lib/lutaml/uml/parsers/dsl_preprocessor.rb

+            path_to_file = File.join(include_root,
+                                     path_to_file)
+          end
+          File.read(path_to_file).split("\n").map do |line|


Shadowing outer local variable - line.

kwkwan · 2025-02-27T01:16:27Z

Hi @ronaldtse,

Please do not merge this PR at this moment as I found some strange performance problem and bugs.

For performance problem, the time for running all lutaml specs is about 3m47s, which is 8m16s before. So, it seems faster.

HOWEVER, if I run the specs of metanorma-plugin-lutaml with the NEW lutaml, it takes 48m. if I run the specs of metanorma-plugin-lutaml with the OLD lutaml, it takes 20m. It is slower than before.

Moreover, when I ran the specs of metanorma-plugin-lutaml even with the OLD lutaml, some Liquid render error: Liquid error: internal were occurred, which were not occurred at the previous time when I ran the specs. As I ran bundle update before running the specs, I thought some underlying gems causing these errors.

ronaldtse · 2025-02-27T01:18:15Z

For NEW and OLD lutaml, can you determine versions they are?

This will be helpful for @HassanAkbar in profiling/debugging. Thanks!

kwkwan · 2025-02-27T01:51:23Z

For NEW and OLD lutaml, can you determine versions they are?

This will be helpful for @HassanAkbar in profiling/debugging. Thanks!

OLD lutaml means v0.9.28.

NEW lutaml means the changes in the branch simplify-xmi-to-liquid.

ronaldtse · 2025-02-27T03:26:09Z

HOWEVER, if I run the specs of metanorma-plugin-lutaml with the NEW lutaml, it takes 48m. if I run the specs of metanorma-plugin-lutaml with the OLD lutaml, it takes 20m. It is slower than before.

Moreover, when I ran the specs of metanorma-plugin-lutaml even with the OLD lutaml, some Liquid render error: Liquid error: internal were occurred, which were not occurred at the previous time when I ran the specs. As I ran bundle update before running the specs, I thought some underlying gems causing these errors.

Actually, it's possible that the metanorma-plugin-lutaml specs are running faster with OLD lutaml because there are Liquid errors leading to some skipped code. If the current lutaml implementation passes all specs, I prefer to merge first and then address any performance issues after. Correctness is more important.

kwkwan · 2025-02-27T03:30:44Z

I found the area in lutaml that causes the Liquid internal error. Let me add some fixes first.

ronaldtse · 2025-02-27T03:32:22Z

lib/lutaml/xmi/liquid_drops/cardinality_drop.rb

      def initialize(model) # rubocop:disable Lint/MissingSuper
        @model = model
      end

      def min
-        @model[:min]
+        return @model[:min] if @model.is_a?(Hash)


Is this for compatibility with the old approach?

No, it is not for compatibility.

ronaldtse · 2025-02-27T03:33:01Z

lib/lutaml/xmi/liquid_drops/constraint_drop.rb

      def initialize(model) # rubocop:disable Lint/MissingSuper
        @model = model
      end

      def name
-        @model[:name]
+        HTMLEntities.new.decode(@model.name)


We should move this to the xmi gem?

@kwkwan can we move this to the xmi gem?

ronaldtse · 2025-02-27T03:34:14Z

lib/lutaml/xmi/parsers/xml.rb

-        # @param with_absolute_path: [Boolean]
-        # @return [Hash]
-        # @note xpath: //uml:Model[@xmi:type="uml:Model"]
-        def serialize_to_hash(xmi_model,


Very nice!!

ronaldtse

Just a few questions.

kwkwan · 2025-02-27T04:58:00Z

HOWEVER, if I run the specs of metanorma-plugin-lutaml with the NEW lutaml, it takes 48m. if I run the specs of metanorma-plugin-lutaml with the OLD lutaml, it takes 20m. It is slower than before.
Moreover, when I ran the specs of metanorma-plugin-lutaml even with the OLD lutaml, some Liquid render error: Liquid error: internal were occurred, which were not occurred at the previous time when I ran the specs. As I ran bundle update be 9E88 fore running the specs, I thought some underlying gems causing these errors.

Actually, it's possible that the metanorma-plugin-lutaml specs are running faster with OLD lutaml because there are Liquid errors leading to some skipped code. If the current lutaml implementation passes all specs, I prefer to merge first and then address any performance issues after. Correctness is more important.

The bugs causing Liquid render error: Liquid error: internal have been fixed.

The performance issue still exists. The time for running specs in metanorma-plugin-lutaml with the changes in the branch simplify-xmi-to-liquid of lutaml is 48m.

ronaldtse · 2025-02-27T05:06:06Z

@kwkwan can you use a ruby profiler to find out where it is being slow?

e.g. https://nipe0324.com/articles/aexymc0-53j3

kwkwan · 2025-02-27T05:07:39Z

@kwkwan can you use a ruby profiler to find out where it is being slow?

e.g. https://nipe0324.com/articles/aexymc0-53j3

Will have a try.

kwkwan · 2025-03-04T08:13:20Z

The commit bdf06fc caches the id name mapping properly. The performance has been improved. The time to run all specs in metanorma-plugin-lutaml is now 12m, which is 20m previously.

The time to run specs of lutaml becomes 3m52s, which is similar as the previous one.

kwkwan added 2 commits February 27, 2025 08:58

Simplify the serialization of xmi to liquid by using Liquid Drop

6e00a29

Fix Rubocop offenses

8000

2e9bd03

kwkwan linked an issue Feb 27, 2025 that may be closed by this pull request

Simplify the serialization of xmi to liquid #135

Closed

hound bot reviewed Feb 27, 2025

View reviewed changes

kwkwan mentioned this pull request Feb 27, 2025

Simplify the serialization of xmi to liquid #135

Closed

ronaldtse rev 8000 iewed Feb 27, 2025

View reviewed changes

ronaldtse reviewed Feb 27, 2025

View reviewed changes

ronaldtse approved these changes Feb 27, 2025

View reviewed changes

Fix Liquid error: internal due to missing options

2bab194

Fix xmi caching - map id to name

bdf06fc

kwkwan mentioned this pull request Mar 4, 2025

Poor performance due to improper caching of the mapping id to name #137

Closed

kwkwan merged commit 7a40324 into main Mar 5, 2025
15 checks passed

kwkwan deleted the simplify-xmi-to-liquid branch March 7, 2025 03:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simplify xmi to liquid #136

Simplify xmi to liquid #136

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Simplify xmi to liquid #136

Simplify xmi to liquid #136

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!