Onto-Med / top-backend

Spring Boot based backend of the TOP Framework
MIT License
0 stars 1 forks source link

191 concept graphs use documents from different source than upload #196

Closed fmatthies closed 7 months ago

fmatthies commented 7 months ago

there are now methods to start the concept graph creation using an external document server and not relying on txt document upload

ChristophB commented 7 months ago

Some commits are not properly rebased from master - e.g. f124dff (has it been cherry-picked?)

When the tests are passing, I'll drop the affected commits and do a rebase afterwards.

fmatthies commented 7 months ago

I did indeed wonder why there were only pom changes when I was rebasing. I didn't intentionally omit commits... don't know why this happened

ChristophB commented 7 months ago

Something is off with the application properties file:

java.lang.IllegalArgumentException: Could not resolve placeholder 'top.documents.document_server.batch_size' in value "${top.documents.document_server.batch_size}"

fmatthies commented 7 months ago

This is weird (wrote about it in the last commit message). Need to look into it.

fmatthies commented 7 months ago

And it just happens during tests because locally my backend runs with the new changes

ChristophB commented 7 months ago

And it just happens during tests because locally my backend runs with the new changes

It looks to me like you are modifying the application context in TopBackendApplication::main, which is not called when the tests are run.

ApplicationContextInitializer should do the trick.

fmatthies commented 7 months ago

Yeah, you're right. But it never happened before (maybe I have defaults in the value annotion everywhere else); that threw me off.

fmatthies commented 7 months ago

It's so frustrating: locally the test are passing. I will look into it some more. Hopefully you aren't spammed with the notifications about the failing tests?

fmatthies commented 7 months ago

@ChristophB So the real reason for all the failures (I fixed them now except for the getDataSources Test in QueryApiDelegateImplTest) seems to be that the test/resources are not used (or old? ones). So the getDataSources Test just finds 2 (and not my nlp/Test_Data_Source_3.yml). Maybe you have some insights?

ChristophB commented 7 months ago

seems to be that the test/resources are not used (or old? ones)

That is not the case. The files there are still used for the tests and the path must be configured in src/test/resources/application.yml. The value for top.documents.data-source-config-dir is wrong and leads to the failing test.

I updated the value in the most recent commit, but this leads to failing document service tests. Looks like something is off with the application context rewrite you have implemented.

Now that the application finds an adapter, it will use it to set the default values right?

fmatthies commented 7 months ago

Not default ones but the ones declared in the adapter. The remaining error resulted from the test documents being indexed not in test_documents but in documents. Hopefully my commit fixes that. Thanks for your help! Don't know how long it would have taken me to arrive at that solution 🤔

fmatthies commented 7 months ago

Some commits are not properly rebased from master - e.g. f124dff (has it been cherry-picked?)

When the tests are passing, I'll drop the affected commits and do a rebase afterwards.

@ChristophB now all tests are passing; finally.

ChristophB commented 7 months ago

Cleaned up the branch and rebased onto master.

fmatthies commented 7 months ago

Done. (I'll update the readme in top-document-query accordingly). Shall I squash and close or do you want to do it?

ChristophB commented 7 months ago

I'll do the merge as there is another unwanted commit 005409b.

ChristophB commented 7 months ago

@fmatthies What is the purpose of this commit? abb59ae It is from January 18 and introduced an unused method.

EDIT: The changes are already included in a previous commit. So I'll just drop that commit.