search

jina-ai / executors

internal-only
Apache License 2.0
31 stars 12 forks source link

fix: run executor normalizer from container #178

Closed alaeddine-13 closed 3 years ago

alaeddine-13 commented 3 years ago

Executor normalizer needs python 3.8 (needs recent AST library) closes https://github.com/jina-ai/executors/pull/178

alaeddine-13 commented 3 years ago

We should use a container

can you re-check ?

tadejsv commented 3 years ago

Ok then change looks good (you might consider setting --user for the container), and we need to then remove previous pip installation of the normalizer. Also, we need to test it somewhere (best place would be the normalizer repo)

cristianmtr commented 3 years ago

For any CI/CD changes we need to test that all Executors still work.

  1. Can you do a mock change in all the folders so that they are all triggered?
  2. Can you enable the jina hub push job (see cd.yml) to run in the PR?
tadejsv commented 3 years ago

@cristianmtr This is something that will take a lot of time, speaking from experience

cristianmtr commented 3 years ago

@cristianmtr This is something that will take a lot of time, speaking from experience

Fixing it afterwards also takes a lot of time. This needs to be tested. Otherwise it slows down any other development in other PRs.

tadejsv commented 3 years ago

Let me create a quickfix then. The problem is that the progress on other PRs is blocked currently, because unit tests do not work due to new version of executor-normalizer

alaeddine-13 commented 3 years ago

dummy changes made and tested some of the executors And then the changes are reverted