Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upload database module updated #295

Merged
merged 6 commits into from
Aug 30, 2024
Merged

upload database module updated #295

merged 6 commits into from
Aug 30, 2024

Conversation

luissian
Copy link
Member

Configuration was changed to have parameters defined for upload_module under upload_module.
Change on code to apply this and fixing errors when upload data to iSkyLIMS.

@saramonzon
Copy link
Member

Looks good @luissian ! Thanks!

But i think you've removed some functionaligy with the LogSum, this is an addition to the "usual" log where we include warnigns and stuff. This other log generates a json summary of warnings and errors found for each sample.

Did you found any error there and that's why you removed it? I think @Shettland can help here, maybe i'm missing something

@luissian
Copy link
Member Author

I decided not to use because I need to write in the log a "Warning" that if something is missing. But according to summary log philosophy warnings messages are showed in terminal if no log file is indicated when running the tool.
On the original log only log.errors are displayed.
Mixing the use of logsum and normal log for log.info is a dirty way. For mi point of view we should use fully in code or logsum or log, but never both.
On the other hand and this one is the main reason is that I do not see the benefit of using for this particular module the log sum.
There is not summary errors for each sample. If for any reason the sample can not be uploaded , it does not look for other errors, just display on the terminal/log and continue with the next.
If i am wrong for sure @Shettland can help :-)

@@ -153,34 +178,38 @@ def get_iskylims_fields_sample(self):
# the field name for the sample
sample_fields[property] = values["field_name"]
except KeyError as e:
self.logsum.add_warning(entry=f"Error mapping ontology {e}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this warning should be added to log sum right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not? If it's an error, why is log.info? And why not add it in the json summary so you can easyly check for sample errors?

else:
# for the ones that do not have ontology label in the sample field
# and have an empty value: sample_fields[key] = ""
logtxt = f"No ontology found for {values.get('field_name')}"
self.logsum.add_warning(entry=logtxt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't know if this is the same as above, but one or the two should be added to the json summary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@Shettland Shettland merged commit 3ce530b into BU-ISCIII:develop Aug 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants