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

Update module_test_python_client.vm.properties #367

Closed bio-boris closed 1 year ago

bio-boris commented 2 years ago

See #347 Called at https://cs.github.com/kbase/kb_sdk/blob/07a0a3ce72060510b58202ce8151f4d4818a81aa/src/java/us/kbase/mobu/initializer/ModuleInitializer.java#L152 and https://cs.github.com/kbase/kb_sdk/blob/07a0a3ce72060510b58202ce8151f4d4818a81aa/src/java/us/kbase/mobu/compiler/TemplateBasedGenerator.java#L206

MrCreosote commented 2 years ago

Why are there almost 100 checks? That seems insane

MrCreosote commented 2 years ago

Making changes to kb_sdk makes me very nervous due to the centrality of the module in KBase, its complexity, and the fact that the tests are unrunnable (except maybe on my magic VM, but IIRC they weren't working the last time I tried, which was years ago).

I assume that whatever change broke things, and caused this change to be necessary, got through because of the lack of tests.

The change itself looks reasonable at face value. The questions I have are

bio-boris commented 2 years ago

1) I did not investigate how and when this bug was introduced 2) As for testing, I did it late at night and should probably redo it, but the testing I did was

(I'm not entirely sure if its possible to change paths and move things around and would need to look into further. Making these changes in the MAKEFILE is not a portable solution as people may have updated the makefile..)

MrCreosote commented 2 years ago

Can we figure out how the bug came to be? That'll also help out re figuring out whether the change is backward compatible or not. It'd be bad if we needed to recompile an old module and it breaks

bio-boris commented 1 year ago

Looking at the git blame, the bug came to be when it got added to the template

https://github.com/kbase/kb_sdk/commit/3fe3e8875bccc2d97ddcca802fede3ff00f506b0#diff-c4bd14f7c977da591d617fdf8e484fd558391135ad1607871c93b549cff53875R19

Hmm, the other one got added https://github.com/kbase/kb_sdk/blob/3fe3e8875bccc2d97ddcca802fede3ff00f506b0/src/java/us/kbase/templates/module_test_python_client.vm.properties way back though so maybe it was during the python2-3 update? Not sure.

I think these changes actually make it more backwards compatible.

Wait this wouldn't affect recompiling old modules, because these steps only happen during init of the module I believe.

MrCreosote commented 1 year ago

It looks to me that whenever the change was made to put all the clients into installed_clients the templates weren't updated correctly and still pointed to the old locations. I think both the commits above are prior to that

MrCreosote commented 1 year ago

One of the files is the python server template; I'm pretty sure that gets recompiled (retemplated?) every time make compile is run

MrCreosote commented 1 year ago

For testing you say you should probably rerun the tests, maybe when you do that you could try recompiling a really old module like DFU and seeing if it still works, recompiling a newer module (not sure what's new) and seeing it it still works, and making a new module and checking it works, along with the tests you outlined above

ialarmedalien commented 1 year ago

I was looking at the "insane" GHA test failures in another kb-sdk PR. Some of the failures are due to GHA issues (timeouts, etc.), but there are also problems with the example module that gets created as part of the GHA tests, specifically that the tool it installed (something from sourceforge) is no longer available.