Conversation
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: attrs (https://github.com/python-attrs/attrs)
- mypy: can't read file 'tests/typing_example.py': No such file or directory
+ mypy: error: cannot read file 'tests/typing_example.py': No such file or directory
|
|
Btw @hauntsaninja |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
I have some questions, mostly about things that I don't fully understand. Otherwise looks good.
| try: | ||
| for id in scc.mod_ids: | ||
| state = graph[id] | ||
| # Extra step below is needed only because we are using local graph. |
There was a problem hiding this comment.
This comment wasn't clear to me. I'm not sure which extra step this refers to (parse_file or the state.tree is None check?).
| # Extra step below is needed only because we are using local graph. | ||
| # TODO: clone options when switching to coordinator graph. | ||
| if state.tree is None: | ||
| state.parse_file() |
There was a problem hiding this comment.
Why are we parsing the file here? Is this related to error processing?
| source_set = BuildSourceSet(sources) | ||
| cached_read = fscache.read | ||
| errors = Errors(options, read_source=lambda path: read_py_file(path, cached_read)) | ||
| # Record import errors so that they sn be replayed by the workers. |
| # We don't need parsed trees in coordinator process, we parse only to | ||
| # compute dependencies. | ||
| state.tree = None | ||
| if not temporary: |
There was a problem hiding this comment.
Should we only have the del statement behind the not temporary condition?
| util.hard_exit(0) | ||
|
|
||
|
|
||
| def serve(server: IPCServer, ctx: ServerContext) -> None: |
There was a problem hiding this comment.
Unrelated: Add a docstring here?
|
|
||
| for id in graph: | ||
| manager.import_map[id] = set(graph[id].dependencies + graph[id].suppressed) | ||
| manager.import_map[id] = graph[id].dependencies_set |
There was a problem hiding this comment.
Is this change related to error processing or the earlier changes to how we process suppressed modules?
| # Cache for transitive dependency check (expensive). | ||
| self.transitive_deps_cache: dict[tuple[int, int], bool] = {} | ||
| # Resolved paths for each module in build. | ||
| self.path_by_id: dict[str, str] = {} |
There was a problem hiding this comment.
Is this because we now delete modules in the middle of a build, or is there another reason for maintaing this here?
| def __init__(self, *, graph: Graph, missing_modules: set[str]) -> None: | ||
| self.graph = graph | ||
| self.missing_modules = missing_modules | ||
| self.from_cache = {mod_id for mod_id in graph if graph[mod_id].meta} |
There was a problem hiding this comment.
It looks like we might calculate this on demand using graph, and it wouldn't be much slower than a set lookup. Why are we storing this separately here?
2 participants
Ref #933
When parallel parsing is ready, we will not run
load_graph()in each worker. So, we need to send early errors (mostly import errors) to the relevant workers. Couple notes here:only_oncemessages are shown only once per worker. It is hard to fix, and I don't think we should really bother. It doesn't affect correctness, and may only slightly increase noise. Btw they were never trulyonly_once, when reading errors from cache we can get multiple occurrences of the same error/note.file.py: error: some messageformat.