update transcript directory structure during export #258
Conversation
edxval/api.py
Outdated
| try: | ||
| # File system should not start from /draft directory. | ||
| static_file_dir = combine(resource_fs._sub_dir.split('/')[1], static_dir) # pylint: disable=protected-access | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
Is there a more specific exception type or set of exception types you could catch here?
edxval/api.py
Outdated
| # File system should not start from /draft directory. | ||
| static_file_dir = combine(resource_fs._sub_dir.split('/')[1], static_dir) # pylint: disable=protected-access | ||
| except Exception: # pylint: disable=broad-except | ||
| logger.exception("VAL Transcript Export: Error creating static directory path for video {}".format(video_id)) |
There was a problem hiding this comment.
Would it be useful to include anything like static_dir or resource_fs in this message?
|
@kdmccormick Thanks, I will address the comments |
DawoudSheraz
force-pushed
the
dsheraz/TNL-7338
branch
from
44c2ae7
to
9e5a013
Compare
|
Thanks for pushing this forward! Out of curiosity does VAL have release automation set up or does this need to be deployed manually? I'm curious when we could test whether this fix addresses old mongo export issues in production. |
I think we need a PR to bump the installed version in |
| # falling back to default path in case of an error | ||
| try: | ||
| # File system should not start from /draft directory. | ||
| static_file_dir = combine(resource_fs._sub_dir.split('/')[1], static_dir) # pylint: disable=protected-access |
There was a problem hiding this comment.
@DawoudSheraz it appears that it's possible for resource_fs to be a WrapFS instead of the expected SubFS at times, and WrapFS doesn't have the private _sub_dir attribute. We're working on developing a fix now, but wondering if you had anything to add regarding the reason behind the particular combination of paths here (second path segment of the directory joined with the static_dir) and what kind of errors it should fallback on (what does KeyError catch here?)
There was a problem hiding this comment.
@DawoudSheraz it appears that it's possible for
resource_fsto be aWrapFSinstead of the expectedSubFSat times, andWrapFSdoesn't have the private_sub_dirattribute. We're working on developing a fix now, but wondering if you had anything to add regarding the reason behind the particular combination of paths here (second path segment of the directory joined with the static_dir) and what kind of errors it should fallback on (what does KeyError catch here?)
Hi. What are the instances in the export where the resource_fs won't be SubFS? As for the join statements, the courses with old key format org/run don't have course as the root directory for export. It was the course run number. After the split, 2nd element in the list is the course run(which is the root directory for old-key format courses). For new courses, the same expression evaluates to course.
There was a problem hiding this comment.
Thanks @DawoudSheraz ! We're still trying to reproduce the issue, so aren't sure exactly yet. I think it's related to one of the override_export_fs functions (here or here). These both set a WrapFS filesystem, and are used in some functions that serialize xblocks (here and here). Perhaps it's only an issue when running the export olx api?
Do think think there will be a simple method that will support both WrapFS and SubFS (either using only public methods defined on both, or a fallback if ._sub_dir is not available)? I'm not familiar with how these virtual filesystems are used, so don't know the practical difference between the two for the purposes used here.
There was a problem hiding this comment.
@DawoudSheraz I investigated further and discovered that indeed a WrapFS is used for use with the olx_rest_api export method. I opened #267 with a fix. :)
`resource_fs` can be set to a WrapFS instance in some cases, notably in override_export_fs, which is used when exporting content via the olx_rest_api. This bug was introduced in openedx#258, which in turn fixed other bugs. So this commit preserves the behaviour from that PR, while ensuring that the original static_file_dir logic is used when a WrapFS is passed (ie. no _sub_dir attr). An alternate option would be to add AttributeError to an except clause, but it feels safer to be more specific and proactive in this case.
…FS (#267) * Fix create_transcripts_xml when resource_fs is WrapFS `resource_fs` can be set to a WrapFS instance in some cases, notably in override_export_fs, which is used when exporting content via the olx_rest_api. This bug was introduced in #258, which in turn fixed other bugs. So this commit preserves the behaviour from that PR, while ensuring that the original static_file_dir logic is used when a WrapFS is passed (ie. no _sub_dir attr). An alternate option would be to add AttributeError to an except clause, but it feels safer to be more specific and proactive in this case.
Fix export for old-style/course/ids
TNL-7338
Description
VAL transcript export in static always expects directory structure to be
course/static. This is valid for new mongo courses but for old mongo courses, the sub-directory is notcoursebut based on the course run. This resulted in export failure for old mongo courses if the transcript was not published. This fix is an enhancement to #255 which fixes the issue for new courses but not old ones.