PASTA-ELN / pasta-eln

PASTA-ELN with new frontend
https://pasta-eln.github.io/pasta-eln/
Other
8 stars 2 forks source link

refactor(dataverse): move initialize_database to backend.py #299

Closed jmurugan-fzj closed 4 months ago

jmurugan-fzj commented 4 months ago
SteffenBrinckmann commented 4 months ago

@jmurugan-fzj can you please try to move the invocation of the setup function after the commented out line 322 and before the return

jmurugan-fzj commented 4 months ago

@jmurugan-fzj can you please try to move the invocation of the setup function after the commented out line 322 and before the return

  • that is where the data-hierarchy was before created. After all the version changes in the data hierarchy, it became unnecessary, but it is still called and it should not cause any issues with the installation test

@SteffenBrinckmann Did you mean "db_api.initialize_database()" by the setup function?

Also I do not see any commented out line 322 in backend.py, or have you meant any other file? You could be more specific by giving comments on exact lines, to do this:

image

SteffenBrinckmann commented 4 months ago

@jmurugan-fzj Sorry, I meant file installationTools.py and in function dataHierarchy()

jmurugan-fzj commented 4 months ago

@jmurugan-fzj Sorry, I meant file installationTools.py and in function dataHierarchy()

@SteffenBrinckmann I tried to move initialize_database() to configSetupLinux.py & configSetupWindows.py where the dataHierarchy('install') is invoked, but the problem is that the example data popuilation phase which comes afterward will remove all the initialized design documents and other data from DB!! So I don't think this is the right place to do the initialization!!

image image

jmurugan-fzj commented 4 months ago

@jmurugan-fzj Sorry, I meant file installationTools.py and in function dataHierarchy()

@SteffenBrinckmann initialize_database() has been moved to configSetupLinux.py & configSetupWindows.py, within the installation "Finished" block! This does not interfere with the installLinux github action too, could you please take a look again?

SteffenBrinckmann commented 4 months ago

@jmurugan-fzj Please remind me: what was the reason for not putting it behind initView for instance after database.py: line 45: self.initDocTypeViews( configuration['tableColumnsMax'] ) self.initGeneralViews() I cannot find that reason in this github issue.

You can also leave it where you have it right now, but it would make sense to have it after initView

jmurugan-fzj commented 4 months ago

@SteffenBrinckmann This will be always initialized whenever pasta app is executed right? In that case, I would prefer the initialization and related checks been done only once during the installation, that's the reason to move it out also do not want to mix the two database abstractions presently...

SteffenBrinckmann commented 4 months ago

@jmurugan-fzj Your comments make sense. In base_database_api: line 60: you get the database name from the config. Which makes sense. However, what happens if the user changes it (in GUI change project group), then you would have to re-initialize your api, correct? To truly separate the databases, I would suggest to create a separate database that is only for dataverse things. That initialization could be then exactly where you did it, during installation. Recreating the example data would then not harm it.

jmurugan-fzj commented 4 months ago

@jmurugan-fzj Your comments make sense. In base_database_api: line 60: you get the database name from the config. Which makes sense. However, what happens if the user changes it (in GUI change project group), then you would have to re-initialize your api, correct? To truly separate the databases, I would suggest to create a separate database that is only for dataverse things. That initialization could be then exactly where you did it, during installation. Recreating the example data would then not harm it.

@SteffenBrinckmann The whole implementation is based on the assumption that the "research" db always exists and dataverse use only this specific DB for the data persistence, so you say that this might not be the case? There can be installations where the research DB does not exists at all, if that's the case then it would be better to move the dataverse to a different database all together, otherwise we could still use the existing "research" DB!

SteffenBrinckmann commented 4 months ago

Yes. For example, I have 3 databases. Do you want to do that I'm this PR?

jmurugan-fzj commented 4 months ago

Yes. For example, I have 3 databases. Do you want to do that I'm this PR?

@SteffenBrinckmann So there can be installations done without the creation of "research" DB at all? My understanding was that research DB will be always created during the first installation of PASTA, If this is the case, then it's better to create a dedicated DB for data-verse! Shall we quickly discuss over the call first, I will try to integrate this change as part of a separate PR then!