Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Implement LogicManager and Explanation #46

Merged
merged 64 commits into from
Jun 22, 2023

Conversation

dmikhalin
Copy link
Contributor

@dmikhalin dmikhalin commented Jun 2, 2023

What is the goal of this PR?

We implement LogicManager, Explanation and corresponding tests.

What are the changes implemented in this PR?

We implemented:

  • Rule struct and LogicManager (with BDD steps)
  • Explanation, Explainable and Explainables structs and added explainables field to ConceptMap (with integration tests)
  • BDD steps for reasoner

@typedb-bot
Copy link
Member

typedb-bot commented Jun 2, 2023

PR Review Checklist

Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.


Trivial Change

  • This change is trivial and does not require a code or architecture review.

Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

@dmikhalin dmikhalin changed the title [DRAFT] Implement LogicManager [DRAFT] Implement LogicManager and Explanation Jun 9, 2023
@dmikhalin dmikhalin changed the title [DRAFT] Implement LogicManager and Explanation Implement LogicManager and Explanation Jun 9, 2023
@dmikhalin dmikhalin marked this pull request as ready for review June 9, 2023 16:44
@dmikhalin dmikhalin requested a review from alexjpwalker as a code owner June 9, 2023 16:44
@dmitrii-ubskii dmitrii-ubskii changed the base branch from master to api-rearchitecture June 12, 2023 10:07
Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

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

The logic integration tests would benefit from being rewritten in a more clear way. Other than that, could do with some architectural changes.

src/answer/concept_map.rs Show resolved Hide resolved
src/answer/concept_map.rs Outdated Show resolved Hide resolved
src/answer/concept_map.rs Outdated Show resolved Hide resolved
src/answer/concept_map.rs Outdated Show resolved Hide resolved
src/answer/concept_map.rs Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
@dmikhalin dmikhalin requested a review from dmitrii-ubskii June 15, 2023 16:33
Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

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

The architecture seems solid, but there is still work to do before this can be merged.

src/answer/concept_map.rs Outdated Show resolved Hide resolved
src/connection/network/proto/concept.rs Outdated Show resolved Hide resolved
src/connection/network/proto/concept.rs Outdated Show resolved Hide resolved
src/connection/network/proto/concept.rs Outdated Show resolved Hide resolved
src/connection/network/proto/logic.rs Outdated Show resolved Hide resolved
Comment on lines 185 to 186
.trim_end_matches(['}', ';', ' '])
.trim_start_matches("{")
Copy link
Member

Choose a reason for hiding this comment

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

Why the trimming here? Seems to leak the TypeQL internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can almost remove trimming if we construct and parse the whole rule:

    let rule = parse_rule(
        format!(
            "rule {}: when {} then {}",
            answer_identifiers.get("label").unwrap(),
            answer_identifiers.get("when").unwrap().trim_end_matches(";"),
            answer_identifiers.get("then").unwrap(),
        )
        .as_str(),
    )
    .unwrap();
    let RuleDefinition { label, when, then } = rule;
    label.name == answer.label && when == answer.when && then == answer.then

Is it possible to parse it without trimming at all?

Copy link
Member

Choose a reason for hiding this comment

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

parse_pattern or parse_patterns, depending on whether the then has a trailing semicolon, should be able to parse the then into a conjunction contatining a single variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_patterns works, trimming has been removed.

tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
@dmitrii-ubskii dmitrii-ubskii self-requested a review June 21, 2023 08:19
Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

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

Final polish, I expect, then you can pass it to Josh! I'd suggest running cargo clippy so that it can catch the little things I may have missed.

src/connection/network/proto/concept.rs Outdated Show resolved Hide resolved
tests/behaviour/typeql/steps.rs Outdated Show resolved Hide resolved
tests/behaviour/util.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Outdated Show resolved Hide resolved
tests/integration/logic.rs Show resolved Hide resolved
Copy link
Member

@flyingsilverfin flyingsilverfin left a comment

Choose a reason for hiding this comment

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

Only 1 major request! Good stuff guys!


impl IntoProto<RuleProto> for Rule {
fn into_proto(self) -> RuleProto {
let Self { label, when, then } = self;
Copy link
Member

Choose a reason for hiding this comment

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

In the next piece of work can you introduce 'Label' to be sibling of 'ScopedLabel'? Then we can replace these usages of string labels with an actual Label struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

src/connection/message.rs Show resolved Hide resolved
tests/BUILD Show resolved Hide resolved
tests/behaviour/typeql/steps.rs Outdated Show resolved Hide resolved
tests/behaviour/typeql/steps.rs Outdated Show resolved Hide resolved
Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

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

Looks good!

tests/behaviour/typeql/reasoner/steps.rs Outdated Show resolved Hide resolved
Copy link
Member

@flyingsilverfin flyingsilverfin left a comment

Choose a reason for hiding this comment

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

Looks good to me!! Good job

@dmikhalin dmikhalin merged commit 1f0d894 into typedb:api-rearchitecture Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants