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

Feature/sckan-324 - journey computation #381

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

D-GopalKrishna
Copy link
Contributor

@D-GopalKrishna D-GopalKrishna commented Dec 10, 2024

Jira Link - SCKAN-324

[ONLY BONUS DEVELOPMENT] - Others are not part of this PR and are explained below:
a. I didn't not see any rows that went empty.
b. export of statement preview is not implemented in this PR. Perhaps we can change the name of the card @ddelpiano ?

Task Implementation description:
Refactor the journey computation

  1. persist the journey in the db
  2. re-compute this everytime we change the entities in path builder are changed - adapting PATCH request to save connectivity statements too.

Demo

2024-12-10.21-40-51.mp4

…_entities fields in ConnectivityStatement model and adapt graph_service
…ViewSet to save associated connectivity_statement
Conflicts:
	backend/composer/api/views.py
backend/composer/models.py Outdated Show resolved Hide resolved
backend/composer/api/views.py Outdated Show resolved Hide resolved
backend/composer/migrations/0064_auto_20241208_2313.py Outdated Show resolved Hide resolved

vias = list(Via.objects.filter(connectivity_statement__id=connectivity_statement.id))
destinations = list(Destination.objects.filter(connectivity_statement__id=connectivity_statement.id))
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct())
Copy link
Member

Choose a reason for hiding this comment

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

question: Why did we change from:
origins = list(connectivity_statement.origins.all())
to
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct())

Copy link
Contributor Author

@D-GopalKrishna D-GopalKrishna Dec 18, 2024

Choose a reason for hiding this comment

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

Reason is here in the code - https://github.com/MetaCell/sckan-composer/blob/feature/SCKAN-324/backend/composer/services/graph_service.py#L24

It says:

Exception has occurred: AttributeError       (note: full exception trace is shown but execution is paused at: _run_module_as_main)
'AnatomicalEntity' object has no attribute 'name'
  File "/home/dgk/metacell/sckan-composer/backend/composer/services/graph_service.py", line 24, in generate_paths
    paths.extend(create_paths_from_origin(origin, vias, destinations, [(str(origin.id), origin.name, 0)], destination_layer))
  File "/home/dgk/metacell/sckan-composer/backend/composer/services/graph_service.py", line 166, in compile_journey
    all_paths2 = generate_paths(origins, vias, destinations)
  File "/home/dgk/metacell/sckan-composer/backend/composer/migrations/0067_auto_20241218_0928.py", line 11, in update_journey_fields
    cs.journey_path = compile_journey(cs)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/db/migrations/operations/special.py", line 193, in database_forwards
    self.code(from_state.apps, schema_editor)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/db/migrations/migration.py", line 130, in apply
    operation.database_forwards(
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/db/migrations/executor.py", line 252, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
    state = self.apply_migration(
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/db/migrations/executor.py", line 135, in migrate
    state = self._migrate_all_forwards(
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/commands/migrate.py", line 349, in handle
    post_migrate_state = executor.migrate(
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/base.py", line 96, in wrapped
    res = handle_func(*args, **kwargs)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/base.py", line 448, in execute
    output = self.handle(*args, **options)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/base.py", line 402, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/__init__.py", line 440, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/site-packages/django/core/management/__init__.py", line 446, in execute_from_command_line
    utility.execute()
  File "/home/dgk/metacell/sckan-composer/backend/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/home/dgk/metacell/sckan-composer/backend/manage.py", line 22, in <module>
    main()
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/dgk/miniconda3/envs/composer/lib/python3.9/runpy.py", line 197, in _run_module_as_main (Current frame)
    return _run_code(code, main_globals, None,
AttributeError: 'AnatomicalEntity' object has no attribute 'name'

so when generating the path - we do have the origins, but not the origin.name - which is a property in AnatomicalEntity.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to access the name of an origin with the original code and I was able to:
image

Can you explain with a bit more detail in which conditions the error you shown occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afonsobspinto did you do the check with the API call?

This happens only when we do the migration - possibly something with - apps.get_model('composer', 'ConnectivityStatement') in the migration file (ref - 0067 auto migration file). Also if you notice this is only for the @properties and not for the fields.
So if you put a breakpoint to the above mentioned code, and then run the migration to db - this should cause the error.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, during migrations django uses historical models that don't include python level constructs like @Property.
I think the new instruction works, but I would remove the .distinct(), I don't think it's needed in our context

backend/composer/models.py Outdated Show resolved Hide resolved

vias = list(Via.objects.filter(connectivity_statement__id=connectivity_statement.id))
destinations = list(Destination.objects.filter(connectivity_statement__id=connectivity_statement.id))
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct())
Copy link
Member

Choose a reason for hiding this comment

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

I tried to access the name of an origin with the original code and I was able to:
image

Can you explain with a bit more detail in which conditions the error you shown occurs?

Copy link
Member

@afonsobspinto afonsobspinto left a comment

Choose a reason for hiding this comment

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

Looking better.

Would it be possible to also change the admin page to not show or keep the journey path as read only?

image

journey_paths = get_journey_path_from_consolidated_paths(
consolidated_paths)
Via = apps.get_model('composer', 'Via')
vias = list(Via.objects.filter(
Copy link
Member

Choose a reason for hiding this comment

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

question: Why do we need to fetch the vias here? can't we rely entirely on the new journey paths attribute created?

entities = []
Via = apps.get_model('composer', 'Via')
vias = list(Via.objects.filter(
Copy link
Member

Choose a reason for hiding this comment

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

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.

2 participants