search

kbase / kb_sdk

Build and test new apps for the KBase platform
http://kbase.github.io/kb_sdk_docs
MIT License
26 stars 32 forks source link

Sct 1585: move installed clients to a the installed_clients dir #326

Closed JamesJeffryes closed 5 years ago

scanon commented 5 years ago

Have you tested this?

JamesJeffryes commented 5 years ago

Yup, works for Java, Python and Perl. Once we get all these changes merged in I'll want to test with some dynamic services

jayrbolton commented 5 years ago

Maybe we could add installed_clients/ to the python path so we don't have to type installed_clients.MyClient every time. Is there a way to do that with the sdk? Not a big deal though.

JamesJeffryes commented 5 years ago

I think we'd have to update the PYTHONPATH environmental variable in the Dockerfile Right now it's pretty clear that the /lib folder is in the root. I'd rather avoid this can of worms because we'd probably need to do the same for Perl

jayrbolton commented 5 years ago

Yeah let's not touch that. More explicit package path could be considered a good thing. So this seems fine for new apps but might be weird for existing apps when you do a new install of a submodule. I can't think of any outright problems, but have you tested this with an existing app?

JamesJeffryes commented 5 years ago

Sorry missed the second part of this comment. For an existing module, the new clients would go in installed_clients while the old ones would stay put unless we wanted to delete & reinstall them (and update the imports). It's not ideal but I don't think it's a big problem.

jayrbolton commented 5 years ago

I'll work on testing locally this morning and then merging

Ideally these should be gitignored, and are installed from a name and version when you pull and run methods. But maybe that is more of a challenge for a new rewrite

jayrbolton commented 5 years ago

I tested this locally, it looks good to me. We'll have to remember to update the documentation when we deploy this. Maybe we can make a few PRs in very common utils (eg. DataFileUtil, AssemblyUtil) as well as the demo app to use this new system, so it's not confusing

jayrbolton commented 5 years ago

Follow up tasks for when this gets deployed: