Implement a tool to query ORAssistant #6226

palaniappan-r · 2024-11-24T12:31:44Z

Implemented the askbot and set_bothost commands under the ora tool. They can be used to make queries to ORAssistant, a LLM based conversational assistant for OpenROAD

`askbot`

This command takes an argument and passes it as a query to the hosted ORAssistant backend, by making a POST request using the libcurl library.

`ora_init`

The ora_init command sets up user consent for using the ORAssistant and configures the host URL (if a local version is preferred).

Signed-off-by: Palaniappan R <[email protected]>

rovinski · 2024-11-24T22:46:40Z

Why can't this run locally as an add-on rather than sending requests over the internet?

palaniappan-r · 2024-11-25T10:31:18Z

Why can't this run locally as an add-on rather than sending requests over the internet?

If the user chooses to the application locally, they'll have to follow instructions on ORAssistant's README, spin the backend server up, and use the set_bothost command to have the askbot command query the local server instead.

Currently, askbot queries a hosted version of ORAssistant by default, but I can change this if necessary.

Signed-off-by: Palaniappan R <[email protected]>

rovinski · 2024-11-27T04:46:47Z

Is there a data control plan or disclaimer if users are sending data over the internet? GDPR may even come into play here.

Signed-off-by: Palaniappan R <[email protected]>

luarss

Just some nits, no comments on code end

luarss · 2024-12-24T14:12:01Z

docker/Dockerfile.builder

@@ -18,4 +18,4 @@ WORKDIR /OpenROAD

 ENV PATH=${LOCAL_PATH}:${PATH}

-RUN ./etc/Build.sh -compiler=${compiler} -threads=${numThreads} -deps-prefixes-file=${depsPrefixFile}
+RUN ./etc/Build.sh -compiler=${compiler} -threads=${numThreads} -deps-prefixes-file=${depsPrefixFile}


why is there a change here?

luarss · 2024-12-24T14:12:14Z

etc/DependencyInstaller.sh

@@ -579,7 +583,7 @@ _installDebianPackages() {
        unzip \
        wget \
        zlib1g-dev
-
+        


stray change?

luarss · 2024-12-24T14:13:06Z

src/ora/src/Ora.tcl

@@ -0,0 +1,43 @@
+# Copyright (c) 2024, The Regents of the University of California


I've updated it now.

Signed-off-by: Palaniappan R <[email protected]>

luarss · 2025-01-05T02:21:03Z

@palaniappan-r no further comments from me, pending @maliberty and @vvbandeira review.

luarss · 2025-01-05T16:53:52Z

On second thought, I think we need to mark this as draft because the permalink is not available. I will discuss more with @dralabeing and @vvbandeira.

gadfort

There is no README for this

gadfort · 2025-01-05T21:09:06Z

src/ora/src/Ora.cc

+  hostUrl
+      = "https://bursting-stallion-friendly.ngrok-free.app/graphs/"
+        "agent-retriever";
+  localDirPath = std::string(getenv("HOME")) + "/.local/share/openroad";


why not make a helper function to generate this path since it's used in multiple locations

gadfort · 2025-01-05T21:10:14Z

src/ora/src/Ora.cc

+  if (!localDir) {
+    logger_->info(utl::ORA, 112, "Creating ~/.local/share/openroad directory.");
+    std::string mkdirCmd = "mkdir -p " + localDirPath;
+    int ret = system(mkdirCmd.c_str());


wouldn't it be better to use: https://en.cppreference.com/w/cpp/filesystem/create_directory instead of generating a system call (something I think we could avoid)

gadfort · 2025-01-05T21:11:39Z

src/ora/src/Ora.cc

+  }
+}
+
+void Ora::setConsent(const char* consent)


since you are checking for just a "y" for yes, I think you need to verify that this consent string matches your expectations.

gadfort · 2025-01-05T21:12:07Z

src/ora/src/Ora.tcl

+}
+
+proc set_consent { consent } {
+  puts "Setting ORAssistant consent to yes."


what is I said "no"?

gadfort · 2025-01-05T21:12:55Z

src/ora/test/regression_tests.tcl

+record_tests {
+  set_bothost1
+  set_consent1
+  askbot1
+}


all the testing needs to be moved to the CTest as this was removed.

I've moved the tests, but still have to figure out a way to start the python server (to mock API responses for tests). Previously, I started the Python server within the regression bash script itself. Could you suggest ways to handle this directly in CTest?

gadfort · 2025-01-05T21:13:48Z

src/ora/src/Ora.tcl

+  ora::set_bothost $hostUrl
+}
+
+proc set_consent { consent } {


set_consent is an awfully generic name, it might be better as set_ora_consent

I've moved this to ora_init.

rovinski · 2025-01-06T07:07:44Z

The request for consent is a bit better than the state before, but there are still a few things I would want to see change:

Default to a locally hosted copy first, or at least explicitly ask the user if it's okay to use the web-hosted version
Make it explicit that only the data the user types will be sent outside the application, no other user or design data.
Point to directions on how to set up a locally hosted copy on first use
Don't require consent for anything other than the web-hosted copy (not only because it's not needed, but because safety guarantees can't be provided)
There needs to be some kind of versioning for the consent. If there were to be a new version in the future which has a different kind of privacy policy, then you can't rely on an old consent file with a new version of OpenROAD.

Signed-off-by: Palaniappan R <[email protected]>

palaniappan-r · 2025-01-19T08:36:20Z

The request for consent is a bit better than the state before, but there are still a few things I would want to see change:

1. Default to a locally hosted copy first, or at least explicitly ask the user if it's okay to use the web-hosted version

2. Make it explicit that only the data the user types will be sent outside the application, no other user or design data.

3. Point to directions on how to set up a locally hosted copy on first use

4. Don't require consent for anything other than the web-hosted copy (not only because it's not needed, but because safety guarantees can't be provided)

5. There needs to be some kind of versioning for the consent. If there were to be a new version in the future which has a different kind of privacy policy, then you can't rely on an old consent file with a new version of OpenROAD.

The askbot command now requires a (y/n) consent, where y is for the web-hosted version and n is for the locally hosted version. Both the consent and the local host URL can be configured using the ora_init command.

rovinski · 2025-01-20T06:30:02Z

That doesn't really make sense - it shouldn't need to ask for consent if not connecting to the OpenROAD web-hosted version. It could be connected to another web-hosted version, in which case, it shouldn't display the message. Not only because the user wouldn't need to consent, but also the other web-hosted version may not have a privacy policy and so the consent message shouldn't claim so.

What about the other concerns?

palaniappan-r · 2025-01-20T07:04:16Z

That doesn't really make sense - it shouldn't need to ask for consent if not connecting to the OpenROAD web-hosted version. It could be connected to another web-hosted version, in which case, it shouldn't display the message. Not only because the user wouldn't need to consent, but also the other web-hosted version may not have a privacy policy and so the consent message shouldn't claim so.

What about the other concerns?

When the askbot command is run for the first time, it displays instructions pointing to a setting up a local version. It then checks if a local url has been provided, subsequent pointing to the ora_init command to configure this. If my earlier comment wasn't clear, only this message is displayed for askbot's first use, not the consent message.

As for the consent management, it is being handed with the ora_init command, with y required for using the official web hosted version (the consent message is only displayed for this). The same command is used to connect the app to a local version. This currently requires the consent to be explicitly set to n, but I can make this automatic if need be.

I've explicitly mentioned that only the data typed-in will be sent outside the application in the consent message as well. I hope this clears your concerns, please let know if there's anything else to be addressed.

rovinski · 2025-01-20T10:08:58Z

Ok thanks, that's much clearer. I don't think using a local copy should require consent to be set to "n". If a user wants to switch between web and local they should be able to without having to constantly grant and revoke consent.

My only other concern would be a version number for the consent to make sure that in the future if the requirements ever change, the user can be reprompted. Something like

version=1
consent=y

then if version 2 comes along which requires different permissions, the user will need to re-consent.

palaniappan-r and others added 25 commits October 21, 2024 14:36

ora: add basic orassistant api call command

fdd34df

Signed-off-by: Palaniappan R <[email protected]>

ora: add boost json lib

13453ac

Signed-off-by: Palaniappan R <[email protected]>

ora: remove context flag

ad67b3a

Signed-off-by: Palaniappan R <[email protected]>

ora: remove context flag, use boost json to parse response

59108fd

Signed-off-by: Palaniappan R <[email protected]>

ora: remove context flag

c03c44d

Signed-off-by: Palaniappan R <[email protected]>

ora: install boost json in docker container

0dd9809

Signed-off-by: Palaniappan R <[email protected]>

ora: add tool entry in logger

972e8d8

Signed-off-by: Palaniappan R <[email protected]>

ora: add set_bothost command

3608133

Signed-off-by: Palaniappan R <[email protected]>

ora: save bothost url to file

11e1fb4

Signed-off-by: Palaniappan R <[email protected]>

Merge branch 'The-OpenROAD-Project:master' into master

581f947

ora: update dockerfile

a885998

Signed-off-by: Palaniappan R <[email protected]>

ora: revert builder dockerfile

68096d7

Signed-off-by: Palaniappan R <[email protected]>

ora: add curl to dependency installer script

119617e

Signed-off-by: Palaniappan R <[email protected]>

ora: add curl and openssl to dependency installer script

ec4c653

Signed-off-by: Palaniappan R <[email protected]>

style: fix formatting

a5c7e96

Signed-off-by: Palaniappan R <[email protected]>

ora: remove default tests

a167887

Signed-off-by: Palaniappan R <[email protected]>

Merge branch 'The-OpenROAD-Project:master' into master

504d82c

Merge branch 'The-OpenROAD-Project:master' into master

fb30cf7

ora: remove log statement

4cf083c

Signed-off-by: Palaniappan R <[email protected]>

ora: add tests

72de8cd

Signed-off-by: Palaniappan R <[email protected]>

ora: add mock API server initialization in regression tests

e64dd2b

Signed-off-by: Palaniappan R <[email protected]>

ora: replace FastAPI mock server with HTTPServer implementation

68e8156

Signed-off-by: Palaniappan R <[email protected]>

ora: add regression test script definition for 'ora' tool

056562f

Signed-off-by: Palaniappan R <[email protected]>

ora: update copyright year to 2024

c44816c

Signed-off-by: Palaniappan R <[email protected]>

ora: update copyright year to 2024 and remove host url file

d2ac50f

Signed-off-by: Palaniappan R <[email protected]>

palaniappan-r and others added 2 commits November 26, 2024 00:16

ora: use dev versions of openssl and curl

c2b240c

Signed-off-by: Palaniappan R <[email protected]>

Merge branch 'The-OpenROAD-Project:master' into master

0d310e4

palaniappan-r added 2 commits December 19, 2024 19:43

ora: add consent management and local directory checks

a29904d

Signed-off-by: Palaniappan R <[email protected]>

ora: update regression tests

e367e39

Signed-off-by: Palaniappan R <[email protected]>

palaniappan-r marked this pull request as ready for review December 21, 2024 03:14

luarss requested review from maliberty and vvbandeira December 24, 2024 14:11

luarss reviewed Dec 24, 2024

View reviewed changes

palaniappan-r and others added 3 commits December 27, 2024 20:42

Merge branch 'The-OpenROAD-Project:master' into master

efb1e27

ora: update copyright year to 2025

408c945

Signed-off-by: Palaniappan R <[email protected]>

ora: fix stray changes

b80a499

Signed-off-by: Palaniappan R <[email protected]>

palaniappan-r requested a review from luarss January 4, 2025 18:26

Merge branch 'The-OpenROAD-Project:master' into master

dc94695

palaniappan-r marked this pull request as draft January 5, 2025 18:14

gadfort requested changes Jan 5, 2025

View reviewed changes

palaniappan-r and others added 10 commits January 10, 2025 13:42

Merge branch 'The-OpenROAD-Project:master' into master

807a93e

refactor: get local directory path from a helper function

d010ae1

Signed-off-by: Palaniappan R <[email protected]>

refactor: improve local directory path handling with filesystem

518e4e1

Signed-off-by: Palaniappan R <[email protected]>

ora: validate consent input and rename related functions

6e84049

Signed-off-by: Palaniappan R <[email protected]>

ora: rename functions for setting host and consent in Ora.tcl

d8a4fbc

Signed-off-by: Palaniappan R <[email protected]>

ora: enhance consent handling and local host configuration

49c729a

Signed-off-by: Palaniappan R <[email protected]>

docs: add README for ORAssistant with command descriptions and usage

642ec40

Signed-off-by: Palaniappan R <[email protected]>

ora: update tests

1b60fc4

Signed-off-by: Palaniappan R <[email protected]>

ora: move tests to ctest

17c1eb3

Signed-off-by: Palaniappan R <[email protected]>

ora: fix typo

6cbccac

Signed-off-by: Palaniappan R <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a tool to query ORAssistant #6226

Implement a tool to query ORAssistant #6226

palaniappan-r commented Nov 24, 2024 •

edited

Loading

rovinski commented Nov 24, 2024

palaniappan-r commented Nov 25, 2024

rovinski commented Nov 27, 2024

luarss left a comment

luarss Dec 24, 2024

palaniappan-r Dec 27, 2024

luarss Dec 24, 2024

luarss Dec 24, 2024

palaniappan-r Dec 27, 2024

luarss commented Jan 5, 2025

luarss commented Jan 5, 2025

gadfort left a comment

gadfort Jan 5, 2025

gadfort Jan 5, 2025

gadfort Jan 5, 2025

gadfort Jan 5, 2025

gadfort Jan 5, 2025

palaniappan-r Jan 19, 2025

gadfort Jan 5, 2025

palaniappan-r Jan 19, 2025

rovinski commented Jan 6, 2025

palaniappan-r commented Jan 19, 2025

rovinski commented Jan 20, 2025 •

edited

Loading

palaniappan-r commented Jan 20, 2025

rovinski commented Jan 20, 2025

		@@ -0,0 +1,43 @@
		# Copyright (c) 2024, The Regents of the University of California

Implement a tool to query ORAssistant #6226

Are you sure you want to change the base?

Implement a tool to query ORAssistant #6226

Conversation

palaniappan-r commented Nov 24, 2024 • edited Loading

askbot

ora_init

rovinski commented Nov 24, 2024

palaniappan-r commented Nov 25, 2024

rovinski commented Nov 27, 2024

luarss 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

luarss commented Jan 5, 2025

luarss commented Jan 5, 2025

gadfort 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

rovinski commented Jan 6, 2025

palaniappan-r commented Jan 19, 2025

rovinski commented Jan 20, 2025 • edited Loading

palaniappan-r commented Jan 20, 2025

rovinski commented Jan 20, 2025

palaniappan-r commented Nov 24, 2024 •

edited

Loading

`askbot`

`ora_init`

rovinski commented Jan 20, 2025 •

edited

Loading