fix: add automount folder when session creation occurs #588
Conversation
Codecov Report
@@ Coverage Diff @@
## main #588 +/- ##
==========================================
- Coverage 48.42% 48.41% -0.02%
==========================================
Files 53 53
Lines 9131 9136 +5
==========================================
+ Hits 4422 4423 +1
- Misses 4709 4713 +4
Continue to review full report at Codecov.
|
| # Fast-path for empty requested mounts | ||
| if not accessible_vfolders: | ||
| return [] | ||
| from icecream import ic |
There was a problem hiding this comment.
is this used for debugging? ic is used on 559 line!
There was a problem hiding this comment.
There was a problem hiding this comment.
My mistakes. I'll fix it right away.
| for vfolder in accessible_vfolders: | ||
| if vfolder['name'] == requested_vfolder_names[key]: | ||
| break | ||
| else: | ||
| raise VFolderNotFound(f"VFolder {vfolder_name} is not found or accessible.") | ||
| if vfolder['name'] not in requested_vfolder_names: | ||
| raise VFolderNotFound(f"VFolder {vfolder_name} is not found or accessible.") |
There was a problem hiding this comment.
accessible_vfolder_names = set(vfolder['name'] for vfolder in accessible_vfolders)
for key, vfolder_name in requested_vfolder_names.items():
if vfolder_name not in accessible_vfolder_names:
raise VFolderNotFound(f"VFolder {vfolder_name} is not found or accessible.")
...
how about this?
rather iterating all accessible_vfolders, using comprehension seems clear
There was a problem hiding this comment.
Hmm, there seems to be a reason for iterating whole accessible_vfolders. As you can see here, the conditional statement(if clause) breaks and those "items"(which is assigned to vfolder) in accessible_vfolders used from here to the end of the function prepare_vfolder_mounts.
IMHO, replacing accessible_vfolders with accessible_vfolder_names should be reconsidered.
There was a problem hiding this comment.
I think Sanghun's suggestion is good. The original inner for-loop is a boilerplate.
| if not accessible_vfolders: | ||
| return [] |
There was a problem hiding this comment.
I think we should handle this case as well.
Instead of directly applying my suggestion, could we simplify the matching?
| if not accessible_vfolders: | |
| return [] | |
| if not accessible_vfolders: | |
| if requested_vfolder_names: | |
| raise VFolderNotFound(f"VFolder {requested_vfolder_names[0]} is not found or accessible.") | |
| else: | |
| return [] |
| for vfolder in accessible_vfolders: | ||
| if vfolder['name'].startswith('.'): | ||
| requested_vfolder_names.setdefault(vfolder['name'], vfolder['name']) | ||
| requested_vfolder_subpaths.setdefault(vfolder['name'], '.') |
| for vfolder in accessible_vfolders: | ||
| if vfolder['name'].startswith('.'): | ||
| requested_vfolder_names.setdefault(vfolder['name'], vfolder['name']) | ||
| requested_vfolder_subpaths.setdefault(vfolder['name'], '.') |
| for vfolder in accessible_vfolders: | ||
| if vfolder['name'] == requested_vfolder_names[key]: | ||
| break | ||
| else: | ||
| raise VFolderNotFound(f"VFolder {vfolder_name} is not found or accessible.") | ||
| if vfolder['name'] not in requested_vfolder_names: | ||
| raise VFolderNotFound(f"VFolder {vfolder_name} is not found or accessible.") |
There was a problem hiding this comment.
I think Sanghun's suggestion is good. The original inner for-loop is a boilerplate.
This PR resolves the issue raised on backend.ai-webui#1302.