search

tshu-w / lit-doccano

Apache License 2.0
1 stars 1 forks source link

Lightning Gallery - engineering review [lit-doccano component] #1

Open manskx opened 2 years ago

manskx commented 2 years ago

Thanks for submitting your component to Lightning Gallary. @tshu-w

That's a powerful component. There are a few points that will be nice to address before the component is added to the gallery.

1- I couldn't install the component on fresh env, seems doccano==1.8.0 conflicts with lightning 

aiobotocore 2.1.2 requires botocore<1.23.25,>=1.23.24, but you have botocore 1.27.49 which is incompatible.

2- Also the dependencies should be added in the setup.py so they get installed when installing the component.

3- In the component I noticed that the username and password are hardcoded, can we improve that? maybe as inputs or using environment variables.

4- Could you add an e2e test for your example app as part of the CI ? You can get some inspiration from here: https://github.com/Lightning-AI/lightning/tree/master/tests/tests_app_examples This is important to ensure the stability of the component :)

5- will be great to add some comments in the example app showing that this component starts a UI and how to configure the component.

If you have any questions, please reach out! we will be happy to assist you to land your component in the Lightning gallery.

cc: @zippeurfou

tshu-w commented 2 years ago

@manskx Hi, sorry for the rushed entry into the Storm Chaser Challenge Series.

I will try to fix this point but not particularly fast. I will let you know when finished.

krishnakalyan3 commented 2 years ago

@tshu-w Looks like we have two problems with this component so far

Issue 1: Update the requirements with the following. This is because of the following issue

cat requirements.txt                                         
Django==4.0.4
doccano==1.8.0

Issue 2: iframe cross-origin issue can be seen in chrome console.

image

This is currently an issue with embedding iframe in lightning. Please see a related issue https://github.com/Lightning-AI/lightning/issues/13520

tshu-w commented 2 years ago

Thanks @krishnakalyan3, I've been busy with experiments recently, I'll take a look when I have time.

robert-s-lee commented 2 years ago

@tshu-w a solution to X-Frame-Options is to remove that using nginx or Caddy. https://github.com/robert-s-lee/lit_label_studio/blob/nginx/lit_label_studio/component.py has sample code used to remove the X-Frame-Options in Label Studio.

Django requires the following env var: 'USE_ENFORCE_CSRF_CHECKS':'false', Nginx conf as follows: https://github.com/robert-s-lee/lit_label_studio/blob/nginx/lit_label_studio/nginx-8080.conf

tshu-w commented 1 year ago

Hi @manskx, after @robert-s-lee excellent PR, we have solved 1 2 3 that you mentioned. But since the current solution requires nginx and a separate venv for doccano, I'm not sure it's appropriate. So 4 and 5 are not done.