Hello Sergey, To your points:
> 1)... > So, by default everything should be same as in master branch and requires > installed java and maven only. I'll make this change but I would like to note, if the python part is to be installed/run, the project will require `conda`. From the comments in the PR, I believe that we have agreed that we would not use the system python of the user (because of obvious conflicts). >2).. > if maven way is prefered - it should be processed with some parameter - like > `mvn clean package/verify/install -some_parameter_use_python` (if it is > possible with maven) - required libs for project: java 11, maven, python, conda (? -is it possible to install it on backstage?) From the comments, I am led to understand that we plan to go the Maven route. I agree with this approach `mvn clean package/verify/install -some_parameter_use_python` . I will try to see if it is possible and make the change. > 3) main current remarks about implementation. > 3.1) We can't keep such progress bar in out installation output > (Look please at attached file) > How to avoid it - another question, but our final installation product can't > print it I guess I think this will be hard to do (the progress bar). Because of the existing state of the maven download plugins. I will still see what the best option is and try to implement it. I agree that it cannot be printed in the final installation. I will work on the remaining points. I'll leave the pull request out there. I'll make the changes and notify you once it is ready. -Rahul Padmanabhan On 8/23/21 2:28 PM, Kamov Sergey wrote: Below my opinion about initial python part refactoring PR - https://github.com/apache/incubator-nlpcraft/pull/18<https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nlpcraft%2Fpull%2F18&data=04%7C01%7Crahul.padmanabhan%40mail.concordia.ca%7Cca0f45094399444889d708d96663c173%7C5569f185d22f4e139850ce5b1abcd2e8%7C0%7C0%7C637653401785403948%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=tQbh0Z2BxmlCneNItHkGunSs1fjwlWqJTuwpld3JU7s%3D&reserved=0> I tried to review how it works and want to shortly describe my remarks which should be fixed in current PR. After we 'll do detailed code review code, etc If somebody has another opinion - please let's discuss. 1) python part should be optional. - required libs for project are just: java 11 and maven. - `mvn clean package/verify/install` should work as in master branch without python stuff. - Apache actions should work https://github.com/apache/incubator-nlpcraft/actions<https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nlpcraft%2Factions&data=04%7C01%7Crahul.padmanabhan%40mail.concordia.ca%7Cca0f45094399444889d708d96663c173%7C5569f185d22f4e139850ce5b1abcd2e8%7C0%7C0%7C637653401785413940%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=FLFwRlxEkEZsYxEcsh4xgeEl0OnzWfbdYFxWKjtzZtQ%3D&reserved=0> - ctx server and spacy are disabled by default. So, by default everything should be same as in master branch and requires installed java and maven only. 2) python stuff usage. there are few python stuff installations ways: - via scripts, - via maven to simplify OS depended issues. if maven way is prefered - it should be processed with some parameter - like `mvn clean package/verify/install -some_parameter_use_python` (if it is possible with maven) - required libs for project: java 11, maven, python, conda (? -is it possible to install it on backstage?) (Suggested flag -Dauto-rm-existing-python-env= - is good thinks I guess, but it is about general approach) 3) main current remarks about implementation. 3.1) We can't keep such progress bar in out installation output (Look please at attached file) How to avoid it - another question, but our final installation product can't print it I guess 3.2) Also we have to try to find some way to avoid warning like >> python/fasttext_module/fasttext/pybind/fasttext_pybind.cc:346:35: warning: comparison of integers of different signs: 'int32_t' (aka 'int') and 'std::__1::vector<long long>::size_type' (aka 'unsigned long') [-Wsign-compare] >> for (int32_t i = 0; i < vocab_freq.size(); i++) { I see that some of these things are out of our control - but we have to do some workarounds, change our approaches etc - we can't provide it to our users (Especially progressbar) 3.3) start_server.sh Duplicated output - should be fixed (it is still here) >>CUDA is not available for Torch. >>CUDA is not available for Torch. >>[2021-08-23 12:52:42]: 80055 DEBUG Registering ctxserver blueprint >>[2021-08-23 12:52:42]: 80057 DEBUG Registering ctxserver blueprint 3.3) server starts too slowly comparing to previous version I think 3.3 and 3.4 - significant issues and should be resolved in this ticket (we can't merge implementations with such degradation) Conclusions: I guess after these 6 points (1, 2, 3.1-3.4) resolving, we can start deep code review and detailed tests on various OS. Other improvements etc can be moved on next tickets. Regards, Sergey
