Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

standardised format of json rule output #569

Merged
merged 17 commits into from
Jan 19, 2025

Conversation

Soph1514
Copy link
Contributor

@Soph1514 Soph1514 commented Jan 6, 2025

Creating standardised json output of rules where every piece of information is a key value pair. At first I tried to use sort_json_object but it led to the keys being sorted not in a logical, consequential order. Separate functionsort_json_ruleswas developed to sort the json objects to allow standardisation of the output.

@Soph1514 Soph1514 changed the base branch from json_rule_format to main January 6, 2025 19:56
@Soph1514 Soph1514 self-assigned this Jan 6, 2025
@Soph1514 Soph1514 marked this pull request as draft January 6, 2025 19:57
Copy link
Collaborator

@niklasdewally niklasdewally left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting is much better!

We might want to keep expressions as raw AST output (as we have now) instead of in the pretty form. This allows us to more closely monitor changes on the internal representation of expressions.

Alternatively, we could store both forms.

@Soph1514
Copy link
Contributor Author

Soph1514 commented Jan 7, 2025

By AST output do you mean the one we have in the "human rule traces" files?

The formatting is much better!

We might want to keep expressions as raw AST output (as we have now) instead of in the pretty form. This allows us to more closely monitor changes on the internal representation of expressions.

Alternatively, we could store both forms.

@ozgurakgun
Copy link
Contributor

I think we should keep the AST form only (as opposed to both) human readable version is in the txt files anyway.

@Soph1514
Copy link
Contributor Author

Soph1514 commented Jan 8, 2025

That makes sense. I just thought you wanted both the last time we talked. I should have asked before proceeding with the pr. Should I uncomment the changes or just delete the branch?

@ozgurakgun
Copy link
Contributor

Sorry I don't think I was being clear. @niklasdewally was suggesting/asking whether we want two versions of the JSON files. I was responding to that, I agree with what he said about using raw AST output in the JSON files instead of pretty. We still want the JSON version you worked on here.

To be concrete, I am suggesting we print the AST instead of a pretty printed version like the following:

    "initial_expression": "(-(3) % -(2) = -(1))",

@Soph1514
Copy link
Contributor Author

Soph1514 commented Jan 8, 2025

O, I got confused and thought we wanted to fully remove the json rules now. I am glad we still need it.

So to be clear, everything is fine, I just to remove pretty printing the value associated with the JSON keys?

@niklasdewally
Copy link
Collaborator

So to be clear, everything is fine, I just to remove pretty printing the value associated with the JSON keys?

Yes: the changes to the layout of the file are good, but instead of printing the model in essence-like syntax, we want it to be like the output of input.expected-rewrite.serialised.json

@ozgurakgun
Copy link
Contributor

Yes, in the above example instead of (-(3) % -(2) = -(1)) it would be something that mentions Mod, Eq, Neg etc.

@Soph1514
Copy link
Contributor Author

[
  {
    "fields": {
      "initial_expression": "Sum(Metadata { clean: false, etype: None }, [Max(Metadata { clean: false, etype: None }, [Atomic(Metadata { clean: false, etype: None }, Reference(UserName(\"a\"))), Atomic(Metadata { clean: false, etype: None }, Reference(UserName(\"b\")))]), Atomic(Metadata { clean: false, etype: None }, Literal(Int(1)))])",
      "new_top": "[AuxDeclaration(Metadata { clean: false, etype: None }, MachineName(0), Max(Metadata { clean: false, etype: None }, [Atomic(Metadata { clean: false, etype: None }, Reference(UserName(\"a\"))), Atomic(Metadata { clean: false, etype: None }, Reference(UserName(\"b\")))]))]",
      "rule_name": "\"flatten_vecop\"",
      "rule_set": "[(\"Minion\", 4400)]",
      "transformed_expression": "Sum(Metadata { clean: false, etype: None }, [Atomic(Metadata { clean: false, etype: None }, Reference(MachineName(0))), Atomic(Metadata { clean: false, etype: None }, Literal(Int(1)))])"
    }
  },
  {

Is this fine? I tried to make the values for initial_expression, transformed_expression and new_top to be in a layered format like in input.expected-rewrite.serialised.json but couldn't figure it out since I am working with strings at the stage of formatting and not Expression/RuleResult struct.

Also the order of the keys is not very logical but I used sort_json_object so it would be standardised. I could implement a custom sorting if needed.

conjure_oxide/src/utils/testing.rs Outdated Show resolved Hide resolved
@Soph1514
Copy link
Contributor Author

I tried the Value approach but it would not compile when I enabled the "unstable" feature. Turns out everything is simpler than we thought. I just experimented printing to the console until I achieved the desired format and plugged it into the trace. I removed json() filter in the subscriber layer (that is what was causing problems) and just pass a formatted json object when calling trace!.

from conjure_oxide/tests/integration/minion_constraints/sumgeq/input-expected-rule-trace.json
[
  {
    "initial_expression": {
      "Geq": [
        {
          "Atomic": [
            {
              "Reference": {
                "UserName": "z"
              }
            },
            {
              "clean": false,
              "etype": null
            }
          ]
        },
        {
          "Sum": [
            [
              {
                "Atomic": [
                  {
                    "Reference": {
                      "UserName": "x"
                    }
                  },
                  {
                    "clean": false,
                    "etype": null
                  }
                ]
              },
              {
                "Atomic": [
                  {
                    "Reference": {
                      "UserName": "y"
                    }
                  },
                  {
                    "clean": false,
                    "etype": null
                  }
                ]
              }
            ],
            {
              "clean": false,
              "etype": null
            }
          ]
        },
        {
          "clean": false,
          "etype": null
        }
      ]
    },
    "rule_name": "introduce_sumgeq",
    "rule_set": [
      [
        "Minion",
        4400
      ]
    ],
    "transformed _expression": {
      "FlatSumGeq": [
        [
          {
            "Reference": {
              "UserName": "x"
            }
          },
          {
            "Reference": {
              "UserName": "y"
            }
          }
        ],
        {
          "Reference": {
            "UserName": "z"
          }
        },
        {
          "clean": false,
          "etype": null
        }
      ]
    }
  },
  {
    "Number of rules applied": 1
  }
]

@Soph1514
Copy link
Contributor Author

Soph1514 commented Jan 18, 2025

trace!(
      target: "rule_engine",
      "{}",
  json!({
      "rule_name": result.rule.name,
      "rule_set": result.rule.rule_sets,
      "initial_expression": serde_json::to_value(initial_expression).unwrap(),
      "transformed _expression": serde_json::to_value(&red.new_expression).unwrap()
  })

@Soph1514 Soph1514 marked this pull request as ready for review January 18, 2025 20:58
@ozgurakgun ozgurakgun merged commit 9befb7f into conjure-cp:main Jan 19, 2025
14 checks passed
niklasdewally added a commit that referenced this pull request Jan 20, 2025
… 2025-01-08)

PR #569 accidentally reverted this change, causing ACCEPT=true failures
on my Mac and some CI failures.
niklasdewally added a commit that referenced this pull request Jan 20, 2025
Reapply 6fde964 (fix(tester): make human rule trace writer blocking,
2025-01-08)

PR #569 accidentally reverted this change, causing ACCEPT=true failures
on my Mac and some CI failures.
niklasdewally added a commit that referenced this pull request Jan 20, 2025
Reapply 6fde964 (fix(tester): make human rule trace writer blocking,
2025-01-08)

PR #569 accidentally reverted this change, causing ACCEPT=true failures
on my Mac and some CI failures.

This commit also adds the previous fix to the new JSON logging layer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants