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

(WIP) Enabling of Knowledge Layer #50

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

chrizmc
Copy link
Collaborator

@chrizmc chrizmc commented Jun 3, 2024

This PR is intended to commit the first basic components that enable a knowledge layer in the CDSP

Copy link
Collaborator

@slawr slawr left a comment

Choose a reason for hiding this comment

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

Hi @claireqiu, thinking to the future it would be good to get a little more information into the commit messages. "add files" forces someone to look into the commit itself to find the nature of the change.

Something like "json-rdf-connector: add data struct and utils headers" or "add data struct and utils headers to json-rdf-connector" would be more informative.

Update: I notice there is good information in the PR comment.

@slawr
Copy link
Collaborator

slawr commented Jun 4, 2024

Great to see this work becoming public and new functionality being added to the Playground itself.

When we first discussed the Knowledge Layer Connector in the May 21st call I remember that we debated whether the Information Layer part should be parallel to the Knowledge Layer part or under it. One point of concern was that in parallel it might be interpreted as an abstraction layer and that whilst it can do that, it would have potentially high performance consequences for latency, high frequency data and ability to access advanced DB queries.

To be honest I do not recall and it is not recorded in the minutes if we came to a conclusion. I do recall saying that one possible approach would be to keep it as presented here and be careful in documenting the circumstances for its use. Also that we might not limit ourselves to it whilst developing the connector as performance was an attribute we were interested in.

So what am I saying? I think we need to be clear on how we are approaching this and the planning should reflect it.

@chrizmc
Copy link
Collaborator Author

chrizmc commented Jun 5, 2024

I understand what you mean. One disadvantage of a structure like this
-knowledge layer
-- information layer
could be that you might think that the information layer can only be used together with the knowledge layer. But in the case of RealmDB (maybe also for IoTDB or other DBs), the websocket IF on the information layer can also be used by a simple UI app or something, without "starting" and using the knowledge layer.

But yes, I agree, we should be clear about how we want to do it....

@slawr
Copy link
Collaborator

slawr commented Jun 6, 2024

I understand what you mean. One disadvantage of a structure like this -knowledge layer -- information layer could be that you might think that the information layer can only be used together with the knowledge layer. But in the case of RealmDB (maybe also for IoTDB or other DBs), the websocket IF on the information layer can also be used by a simple UI app or something, without "starting" and using the knowledge layer.

Agreed. I think you would do something like that if you were communicating that at the current state of development it is specified for the demands of the knowledge layer connector. Moving it later as it evolves. However as you go onto say that might confuse people who would like to use it for other purposes.

I understand that point. Also of course in terms of having a workaround for the fact that Realm is an Embedded Application Database normally bound into a specific application.

But yes, I agree, we should be clear about how we want to do it....

I think for me it is a possible communications issue around explaining the playground and avoidance of people grossly under utilising the tool-set.

I think it's great that this PR already contains some documentation.
Of course doc will always be a challenge.
When I read the root readme for the info layer it authoritatively states the components for the playground info layer are the database handlers and router. It goes on to say data access is through this web socket server and that Realm is the single database supported.
From the viewpoint of the existing BMW UX app development and in the context of the connector I think this is good documentation but it doesn't put it in context for the more general case.
For example:
Does this WS server handle a broad range of use cases? Is it simple single key/value get/set only for example?
How does it handles complex queries?
The Playground presented at the Autumn AMM discussed in part the possibilities of TS DBs, then if this was merged as it is now people could be confused by discussion of data access through this WS and IoTDB is not supported, but a new DB Realm is..

If I'm someone from one of the COVESA BoFs what do I understand about data access in the Playground in reading this?

Of course a balance in doc needs to be struck. We are not a supplier and people need to put some effort in.

I've not thought about it deeply, but one approach could be a large info box at the top putting the rest of the doc in context. This was developed for X,Y, Z requirements, if your requirements are different then you may consider using other means such as directly accessing a component.

Does that make sense?

@chrizmc chrizmc force-pushed the feature/enabling-of-knowledgelayer branch 2 times, most recently from 443a609 to 2407bf3 Compare June 19, 2024 12:04
chrizmc and others added 13 commits June 19, 2024 14:32
…lmdb-handler including some resdme updates

Signed-off-by: Christian Muehlbauer <[email protected]>
Signed-off-by: haonan-qiu <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Christian Muehlbauer <[email protected]>
Signed-off-by: Christian Muehlbauer <[email protected]>
@sschleemilch sschleemilch force-pushed the feature/enabling-of-knowledgelayer branch from 2407bf3 to 0da163a Compare June 19, 2024 12:38
Signed-off-by: Christian Muehlbauer <[email protected]>
Copy link
Collaborator

@slawr slawr left a comment

Choose a reason for hiding this comment

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

It is not a requirement for merging but for the actual code modules you might consider adding SPDX information in a header to allow automation of license handling.

Here is an example: https://github.com/slawr/cdsp/blob/iotdb-udf-grafana/docker/iotdb/Dockerfile

    Use dotenv package to laod environment variables from a .env file into the process.env
    Rename files and update code
    Remove unused file

Signed-off-by: Christian Muehlbauer <[email protected]>
@chrizmc
Copy link
Collaborator Author

chrizmc commented Jul 10, 2024

We will have a look into SPDX, thank's for the hint.

chrizmc added 3 commits July 24, 2024 12:23
Add git ignore for the docker generated files
Add Read method to IoTDBHandler
Rename Data Type Object
Apply format and remove invalid comment
Optimize code to standarize the response between handlers.
Repair documentation for the project configuration
Add anonym data
Manage Sessions
Add Write method add client message data to IoTDB

Signed-off-by: Christian Muehlbauer <[email protected]>
Return messages to all clients when write in IoTDB
Update IoTDB supported fields and documentation
Update read dependencies and send error to clients
Add read messaging in RealmDB
Resolve Ajv warining about fields with multiple types
Add subcribe message handling
Remove unused import
Repair findings

Signed-off-by: Christian Muehlbauer <[email protected]>
Copy link
Collaborator

@slawr slawr left a comment

Choose a reason for hiding this comment

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

Some initial review comments

@@ -11,21 +11,24 @@ Setting up an information layer in the CDSP involves running a [Database-Router]

Please follow installation instructions of the chosen [handler](./handlers/).

## [Installation of Database-Router](./router/README.md#Install)
## Installation of Database-Router
See [hier](./router/README.md#Install) how to install the Database-Router.
Copy link
Collaborator

Choose a reason for hiding this comment

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

'hier' should be 'here'?


## Look out for the Websocket Server message in the console
If you are running the RealmDB handler and you change the value of `Vehicle_Cabin_HVAC_AmbientAirTemperature` in ATLAS cloud (let's say `23`), you should immediately see this line in console:
If you the handler is running and you are subscribed to that element, when you change the value of `CurrentLocation_Longitude` in ATLAS cloud (let's say `-157845.68200000003`), you should immediately see this line in console:
Copy link
Collaborator

Choose a reason for hiding this comment

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

'If you the handler'?

Fix: Remove extra 'dot' from endpoint into update message.
Fix unknown function issue

Signed-off-by: Sebastian Schleemilch <[email protected]>
sschleemilch and others added 5 commits September 6, 2024 16:45
Signed-off-by: Sebastian Schleemilch <[email protected]>
    - Update documentation and remove unused code.
    - Start the websocket client and update docuentation
    - Working synchronous calls
    - First running implementation of websocket_client

Signed-off-by: Andre Wendel <[email protected]>
   -  Add README to add new data source in the information layer

Signed-off-by: Andre Wendel <[email protected]>
@sschleemilch sschleemilch force-pushed the feature/enabling-of-knowledgelayer branch from 635ceac to 7f1ae37 Compare September 26, 2024 07:35
sschleemilch and others added 14 commits September 26, 2024 09:36
Implement functionality to read, validate, and integrate vehicle data points from a configuration file into the knowledge layer, ensuring only supported signals are processed by the system.

Signed-off-by: Sebastian Schleemilch <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
Add helper documentation to system methods and remove outdated README.
Fix read message containing data points.
Merge remote-tracking branch 'origin/feature/enabling-of-knowledgelay

Signed-off-by: Christian Muehlbauer <[email protected]>
Remove unused code
Merge remote-tracking branch 'origin/feature/enabling-of-knowledgelay…
Add missing include to data_types header
Update readmes
Update readme
Fix typo with correct RDFox licence file.

Signed-off-by: Christian Muehlbauer <[email protected]>
Establish Connection to RDFox REST API
Data Store Management
Data Loading in Turtle Format
SPARQL Query Execution
Error Handling
Unit and Integration Tests
Documentation Updates

Signed-off-by: Haonan Qiu <[email protected]>
Signed-off-by: Andre Wendel <[email protected]>
Signed-off-by: Christian Muehlbauer <[email protected]>
…pt backport, because of compatibility issues withiin TS;

Signed-off-by: Andre Wendel <[email protected]>
Signed-off-by: Sebastian Schleemilch <[email protected]>
transform underlines to dots

Signed-off-by: Christian Muehlbauer <[email protected]>
only log if there have been changes
Bugfix: copy tsconfig.json
add a test
Review: small changes
Add configuration variable for polling interval in readme
new unit tests, supress logger in tests
More type safety when converting websocket
Setup ubit tests with jest. write tests for SubscriptionSimulator

Signed-off-by: Christian Muehlbauer <[email protected]>
Copy link
Collaborator

@slawr slawr left a comment

Choose a reason for hiding this comment

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

Flagging an early review comment as the impact is somewhat large

"id": "123", // The VIN
"uuid": "testClient", // The unique client ID
// For reading one
"node": { "name": "Vehicle_Speed" },
Copy link
Collaborator

@slawr slawr Nov 13, 2024

Choose a reason for hiding this comment

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

An early review observation is that in your IoTDB implementation you have chosen to replace VSS dot notation in VSS node/leaf names with underscores to (I assume) integrate and avoid conflicts with the dot notation of the IoTDB data model. Whilst a valid approach (I considered the same when creating the Playground) it is different to the one I choose which quotes the VSS name and which is used in various places in the Playground codebase. For example, the RemotiveLabs feeder, the VISSR integration, examples and in the online documentation. See https://covesa.github.io/cdsp/manuals/apache-iotdb/#integrating-vss-data-into-the-iotdb-data-model

So we need to have an alignment discussion to resolve this.

Signed-off-by: Christian Muehlbauer <[email protected]>
@slawr slawr added enhancement New feature or request task Task not fitting other labels, e.g. project ops labels Nov 26, 2024
…ueries

    Create end to end integration test

    Create a websocket interface to allow dependency injection in order to test the client

    Rename files to use the naming convention

    Add triples to RDF Server

    Add feature to rewirte and mantain the triple output messages

    Correct collection of old triples after update messages

    Correct test to use the acutal observation identifier

    COVESA:Websocket client Integration JSON-to-RDF Converter

    Add variable description

    Update minimum CMake version to 3.25 to have the be compatible with FindBoost Policy CMP0167

    Update CMakeLists for Linux compatibility

    Update Required cmake version because it is required in the serd project

    Code refactoring

    Remove unnecessary check of serd installation, it is not required any more since CMake-Serd is included in the project

    Adapt code to use the CMake-Serd library

    Move triple writer files to the correct project folder

    Fix Code to use the File Handler interface

    Use interface convention

    Transform JSON Data into RDF Triples Using SHACL Shapes and SPARQL Queries

    Remove the test output result

    Check serd library installation

Signed-off-by: q632394 <[email protected]>
…g IoTDB

    added tests
    send subscription status message after subscribe/unsubscribe

Signed-off-by: Christian Muehlbauer <[email protected]>
    Changes due to Review comments
    move authenticateAndConnect() to Session
    aligned test and small changes
    run only 1 instance of SubscriptionSimulator with a seperate client and session.
    open/close session on connect/disconnect per client
    removed function sendMessageToClients()
    1. create a handler for each client, 2. only use WebSocketWithId, 3. log websocket id
    use HandlerCreator to create handler
    first extension of readme
    log timestamp with ms
    check if the received message is an update message
    count messages in queue
    synchronize sent and received messages
    1. convert the signal data and 2. receive responses to prevent backpressure
    new cl argument for element id (e.g. a vehicle id)
    only open websocket once - have extra websocket thread and event loop
    first version: writes to websocket, but not correctly

Signed-off-by: Christian Muehlbauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request task Task not fitting other labels, e.g. project ops
Projects
Development

Successfully merging this pull request may close these issues.

5 participants