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

Changes so that the json log can capture more data, mainly the fields… #531

Merged
merged 8 commits into from
May 15, 2024

Conversation

ggsdc
Copy link
Member

@ggsdc ggsdc commented May 10, 2024

… that are defined on the schema on cornflow
It also makes it so that the dag registry is executed on cornflow service init to make sure that everything is up to date

@ggsdc ggsdc added enhancement New feature or request server Issues relating to the server client Issues relating to the client library labels May 10, 2024
@ggsdc ggsdc added this to the v1.1.0 milestone May 10, 2024
@ggsdc ggsdc requested a review from JaimeSotomayor May 10, 2024 15:11
@ggsdc ggsdc self-assigned this May 10, 2024

log_json = {
**output,
**{
Copy link
Contributor

Choose a reason for hiding this comment

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

¿Mejor en este orden? Yo lo pondría al revés, prevalece lo que venga del output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Si, por que el status se está procesando entonces el status tiene que prevalecer

Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 60.60606% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 78.45%. Comparing base (dc8ac06) to head (1f73d41).

Files Patch % Lines
cornflow-server/cornflow/commands/schemas.py 4.34% 22 Missing ⚠️
cornflow-server/cornflow/cli/service.py 0.00% 1 Missing ⚠️
libs/client/cornflow_client/core/application.py 90.00% 1 Missing ⚠️
libs/client/cornflow_client/raw_cornflow_client.py 83.33% 1 Missing ⚠️
libs/client/cornflow_client/schema/manager.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #531      +/-   ##
===============================================
- Coverage        78.90%   78.45%   -0.45%     
===============================================
  Files              263      258       -5     
  Lines            14860    14393     -467     
===============================================
- Hits             11725    11292     -433     
+ Misses            3135     3101      -34     
Flag Coverage Δ
client-tests 80.66% <92.10%> (+2.81%) ⬆️
server-tests 81.44% <17.85%> (-1.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@JaimeSotomayor JaimeSotomayor left a comment

Choose a reason for hiding this comment

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

Revisaría cambiar el tema de que "status" para los DAG se devuelve un código pero después en cornflow se use "status" como el status en texto y luego "status_code". Tendría más sentido que todo fuera coherente

@ggsdc
Copy link
Member Author

ggsdc commented May 13, 2024 via email

@JaimeSotomayor
Copy link
Contributor

Me refiero al status del solve. En concreto el problema lo veo aquí:

                "status": STATUS_CONV.get(status, "Unknown"),
                "status_code": status,

Para la gente que ha desarrollado el modelo el status es el código numérico. Sin embargo, si se va a la bbdd de cornflow se está guardando el "status" dentro de "status_code". Simplemente digo que eso debería ser coherente

@ggsdc
Copy link
Member Author

ggsdc commented May 13, 2024 via email

@ggsdc ggsdc merged commit 894f4f9 into development May 15, 2024
27 of 28 checks passed
@ggsdc ggsdc deleted the feature/better_log branch May 15, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Issues relating to the client library enhancement New feature or request server Issues relating to the server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants