-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(opentrons-ai-client, opentrons-ai-server): add folders for opentrons-ai #14788
Conversation
|
opentrons-ai-client/src/App.css
Outdated
#root { | ||
max-width: 1280px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider using styled-components like we do in the app for consistency. we can create global styles like this: https://github.com/Opentrons/opentrons/blob/edge/app/src/atoms/GlobalStyle/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all css files are deleted.
"@fontsource/dejavu-sans": "5.0.3", | ||
"@fontsource/public-sans": "5.0.3", | ||
"@opentrons/components": "link:../components", | ||
"i18next": "^19.8.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea to add i18next right off the bat instead of what we did with PD haha
@@ -0,0 +1,2 @@ | |||
# opentrons ai server makefile | |||
# TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this makefile needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it is but looks to be causing a lint error right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really?
Did you run make lint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the command and I didn't see any errors.
Could you share the error you saw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for sharing.
fortunately, that isn't a lint error.
# prerequisite: install dependencies as specified in project setup | ||
make setup | ||
# launch the dev server | ||
make -C opentrons-ai-server dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just double checking that this make -C opentrons-ai-server dev
does not work yet right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet.
server will be added, and we will use python for the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I followed the instructions and opened up the opentrons ai react app 🎉 . Great work so far, everything is very organized in my opinion. Just left some comments and questions but nothing too major.
- [React][] | ||
- [Babel][] | ||
- [Vite][] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a single sentence for each line to know why they are there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the reason is monorepo
|
||
Some important directories: | ||
|
||
- `opentrons-ai-server` — Opentrons AI application's server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Could we provide hyperlink here so that when we click it we go directly there.
|
||
## Copy management | ||
|
||
We use [i18next](https://www.i18next.com) for copy management and internationalization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does internationalisation mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be translated into other languages and that is the reason why we use i18next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean English to Japan for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that is one example.
I think we can gat help from Ed to make readme better before the externa beta testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18next will work like this when we add simple ui for language change and json files for languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could you provide a diagram showing how all the tools and libraries are connected? I want to see how each one is used. If that's not possible, no worries.
opentrons-ai-client/src/assets/localization/en/protocol_generator.json
Outdated
Show resolved
Hide resolved
opentrons-ai-client/src/assets/localization/en/protocol_generator.json
Outdated
Show resolved
Hide resolved
"tipracks_and_labware": "Tip racks and labware: Use names from the Opentrons Labware Library.", | ||
"type_your_prompt": "Type your prompt...", | ||
"well_allocations": "Well allocations: Describe where liquids should go in labware.", | ||
"what_if_you": "What if you don’t provide all of those pieces of information?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since OpenAI cannot follow instruction consistently, this may not happen all the time. So I am happy to remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why everything line by line, why dont we do something like triplet quote in python?
''' '''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is json file and we use eslint.
Yup, I will create a doc for this for engineers. |
…tor.json Co-authored-by: Elyor Kodirov <[email protected]>
…rons-ai (#14788) * feat(opentrons-ai-client, opentrons-ai-server): add folders for opentrons-ai
Overview
opentrons-ai-client: frontend
opentrons-ai-server: backend (content will be added later)
js set up commnad works without any errors and when you run
make -C opentrons-ai-client dev
, the dev server works without any errors and you can access localhost:5173.close AUTH-202, AUTH-203
Test Plan
make teardown-js && make setup-js
make -C opentrons-ai-client dev
Changelog
Review requests
Risk assessment
low