[KED-763] Fix windows bugs for experiment tracking #809
Conversation
rashidakanchwala
changed the title
package/kedro_viz/api/graphql.py
Outdated
| @@ -240,16 +241,21 @@ def get_run_tracking_data( | |||
| all_runs = {} | |||
| for run_id in run_ids: | |||
| run_id = ID(run_id) | |||
| file_path = dataset._get_versioned_path(str(run_id)) | |||
| if Path(file_path).is_file(): | |||
| dataset._version = DatasetVersion(run_id, None) | |||
There was a problem hiding this comment.
What does this line do exactly? Is it to set the load_version to the run_id? I think a short note on what's happening here would be useful for future reference.
There was a problem hiding this comment.
Awesome work! So nice to have this fixed and also be able to get those tests cleaned up 🎉 It's always been a slight irritation in the back of my mind that we never figured that out 😅 I'll be able to sleep at night again now.
Just some very minor comments, and please would it also be possible to clean up test_load_latest_tracking_data in the same way?
package/kedro_viz/api/graphql.py
Outdated
| @@ -21,6 +20,8 @@ | |||
|
|
|||
| import strawberry | |||
| from fastapi import APIRouter | |||
| from kedro.io.core import Version as DatasetVersion | |||
There was a problem hiding this comment.
| from kedro.io.core import Version as DatasetVersion | |
| from kedro.io.core import Version as DataSetVersion |
Is how we normally write it in kedro core. Great idea to alias it in order to distinguish from the kedro-viz Version though 👍
package/kedro_viz/api/graphql.py
Outdated
| logger.warning( | ||
| "`%s` could not be found", dataset._get_versioned_path(str(run_id)) | ||
| ) |
There was a problem hiding this comment.
Minor tidy: move load_path definition to before if dataset.exists() and then change this logging to:
| logger.warning( | |
| "`%s` could not be found", dataset._get_versioned_path(str(run_id)) | |
| ) | |
| logger.warning( | |
| "`%s` could not be found", load_path) | |
| ) |
There was a problem hiding this comment.
@AntonyMilneQB -- when i use load_path for the warning. and then I have to write the load_path function also for the tests. is that ok?
| def example_multiple_run_tracking_catalog_at_least_one_empty_run(example_run_ids): | ||
| def example_multiple_run_tracking_catalog_at_least_one_empty_run( | ||
| example_run_ids, tmp_path | ||
| ): | ||
| # Note - filepath is assigned without using tmp_path as it fails on windows build. |
There was a problem hiding this comment.
We can remove this comment now 🎉
Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
|
Thank you. This is amazing to see fixed. |
| file_path = dataset._get_versioned_path(str(run_id)) | ||
| if Path(file_path).is_file(): | ||
| # Set the load_version to run_id | ||
| dataset._version = DataSetVersion(run_id, None) |
There was a problem hiding this comment.
@rashidakanchwala do you mind helping me understand why it fixed the issue?
There was a problem hiding this comment.
So removing the if Path(file_path).is_file() fixed it because it was returning false.
The other stuff I just changed because Antony suggested we should use load method instead of directly accessing private function.
I did some debugging around why Path(file_path).is_file is returning false when we use Win CMD. Too long to put it here, so I will share with u on discord.
P.S - miss you on slack
Description
Resolves #809
Development notes
When we were developing experiment tracking BE -- there was a known issues of tests failing on win38 due to an issue with windows path.
This was further realised when our Windows users could not see their experiment tracking data.
This issue has now been fixed -- Win38 tests now pass using 'tmp_path'
I have also tested this build on Windows using CMD and the experiment tracking data shows as expected.
@AntonyMilneQB thank you for all your guidance on this ticket <3
QA notes
To test this on Windows CMD. You will need to do the following :-
pip install -e package
cd demo-project
kedro viz --port=4142
Checklist
RELEASE.mdfile