Martin fileinfo cache 2 #2833

schreiberx · 2024-12-19T16:39:26Z

This is the final PR for the updates on caching fparser (and later on psyir).

This update avoids using the same file multiple times for individual modules in each file.

schreiberx · 2024-12-19T16:40:43Z

@arporter That's the final PR for the file info caching stuff.
It's probably best to first get the previous PR in the master.

codecov · 2024-12-19T17:13:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.89%. Comparing base (6afc06a) to head (473e0d2).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2833    +/-   ##
========================================
  Coverage   99.89%   99.89%            
========================================
  Files         359      359            
  Lines       51073    51307   +234     
========================================
+ Hits        51021    51255   +234     
  Misses         52       52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

schreiberx · 2025-01-10T12:37:55Z

@arporter Your turn. Let's try to stay below 50 comments :-)

arporter

Looks good Martin, this is functionality that will be important to have.
There are a couple of places where you appear to be duplicating functionality that we already have so I've asked for clarification.
I've not looked at the tests yet (but these will require updating anyway when you remove the bare asserts from the code).

doc/developer_guide/module_manager.rst

src/psyclone/parse/file_info.py

arporter · 2025-02-17T11:32:13Z

src/psyclone/parse/module_manager.py

+
+    def get_all_module_infos(self) -> List[ModuleInfo]:
+        """
+        Return a list of all module infos


:returns: list of all module infos.

You've missed this one?

arporter · 2025-02-17T11:32:25Z

src/psyclone/parse/module_manager.py

+
+    def get_all_file_infos(self) -> List[FileInfo]:
+        """
+        Return a list of all FileInfo objects


:returns: ...

Thanks. There's no need to duplicate when it's this simple - you can just have the :returns: ... line instead of the initial "Returns a list of ..." text.

arporter · 2025-02-17T11:34:05Z

src/psyclone/parse/module_manager.py

+
+        return list(self._modules.values())
+
+    def get_all_file_infos(self) -> List[FileInfo]:


Am I right in thinking this doesn't do anything other than return information on objects we've already created? If so, I think this should be an @property named all_file_infos.

Not addressed?

arporter · 2025-02-17T11:34:36Z

src/psyclone/parse/module_manager.py

+
+            self._filepath_to_module_info[filepath] = module_info_in_file
+
+    def get_all_module_infos(self) -> List[ModuleInfo]:


c.f. my comment on get_all_file_infos, could this be a property named all_module_infos?

Not addressed?

src/psyclone/parse/module_manager.py

schreiberx · 2025-02-20T11:56:11Z

@arporter Back to you :-)

arporter · 2025-02-28T10:00:47Z

Thanks Martin. There are a few things to look at including a few you seem to have missed last time :-) I note that there is still at least one bare assert in the code but I'll do a proper grep next time around. Please could you also bring up-to-date with master.

arporter

Please see inline comments.

arporter · 2025-02-28T09:07:10Z

src/psyclone/parse/module_info.py

@@ -324,6 +325,22 @@ def get_symbol(self, name: str) -> Union[Symbol, None]:
        except KeyError:
            return None

+    def get_routine_by_name(


This is still outstanding?

arporter · 2025-02-28T09:20:20Z

src/psyclone/parse/module_manager.py

@@ -286,7 +304,7 @@ def load_all_fparser_trees(self, verbose: bool = False) -> None:
            fileinfo: FileInfo
            fileinfo.get_fparser_tree(verbose=verbose)

-    def load_all_psyir_nodes(self, verbose: bool = False) -> None:
+    def create_all_psyir_nodes(self, verbose: bool = False) -> None:
        """
        Routine to load the psyir nodes of all files added


s/load/create/

arporter · 2025-02-28T09:21:36Z

src/psyclone/parse/module_manager.py

+
+    def get_all_module_infos(self) -> List[ModuleInfo]:
+        """
+        Return a list of all module infos


You've missed this one?

arporter · 2025-02-28T09:22:38Z

src/psyclone/parse/module_manager.py

+
+    def get_all_file_infos(self) -> List[FileInfo]:
+        """
+        Return a list of all FileInfo objects


Thanks. There's no need to duplicate when it's this simple - you can just have the :returns: ... line instead of the initial "Returns a list of ..." text.

arporter · 2025-02-28T09:43:18Z

src/psyclone/parse/module_manager.py

+
+        return list(self._modules.values())
+
+    def get_all_file_infos(self) -> List[FileInfo]:


Not addressed?

arporter · 2025-02-28T09:44:05Z

src/psyclone/parse/module_manager.py

+
+            self._filepath_to_module_info[filepath] = module_info_in_file
+
+    def get_all_module_infos(self) -> List[ModuleInfo]:


Not addressed?

arporter · 2025-02-28T09:58:00Z

src/psyclone/parse/module_manager.py

        order of dependencies, i.e. a module which is used by another module
        is listed before the module which uses it.

+        The differences to `get_all_dependencies_recursively` are as follows:


Thanks for updating. However, I still think that with a little bit of work, this method could mostly use get_all_dependencies_recursively - you could pass it a list containing just the module info you're after. Why do you need to preserve the ordering of the list of dependent ModuleInfos?

When doing this, please also remove the bare assert.

SCHREIBER Martin added 10 commits November 30, 2024 21:50

extensions to module manager

0d6e374

added indentation str

2860db2

tests passed

67476c4

minor fixes - tests passed

4e27a05

tweaks

4679c97

u

9073a96

Merge branch 'martin_fileinfo_cache' into martin_fileinfo_cache_2

43d127a

Merge branch 'martin_fileinfo_cache' into martin_fileinfo_cache_2

ce7aa2f

Merge branch 'martin_fileinfo_cache' into martin_fileinfo_cache_2

8f0bd34

Merge branch 'martin_fileinfo_cache' into martin_fileinfo_cache_2

8cd0405

schreiberx requested a review from arporter December 19, 2024 16:39

schreiberx self-assigned this Dec 19, 2024

u

8c2f023

SCHREIBER Martin added 5 commits December 19, 2024 22:28

code coverage

feac0d4

coverage

5cb2491

coverage updates

1afcc96

coverage update

97d833b

u

4baf55d

schreiberx added the ready for review label Dec 22, 2024

SCHREIBER Martin added 6 commits January 8, 2025 21:26

Merge branch 'martin_fileinfo_cache' into martin_fileinfo_cache_2

190b791

flake8 updates...

a2f6567

Merge branch 'master' into martin_fileinfo_cache_2

899ca01

for coverage tests

f5e6d9a

update for PR

ff656b0

u

fc8f751

schreiberx marked this pull request as ready for review January 13, 2025 22:12

Merge branch 'master' into martin_fileinfo_cache_2

0f9cdcd

arporter added under review and removed ready for review labels Feb 17, 2025

Merge branch 'master' into martin_fileinfo_cache_2

473e0d2

arporter requested changes Feb 17, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Feb 17, 2025

SCHREIBER Martin added 2 commits February 20, 2025 12:53

for Andy

0ec2d82

u

23c7c13

schreiberx added ready for review and removed reviewed with actions labels Feb 20, 2025

arporter added under review and removed ready for review labels Feb 28, 2025

arporter requested changes Feb 28, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Martin fileinfo cache 2 #2833

Martin fileinfo cache 2 #2833

schreiberx commented Dec 19, 2024

schreiberx commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

schreiberx commented Jan 10, 2025

arporter left a comment

arporter Feb 17, 2025

arporter Feb 28, 2025

arporter Feb 17, 2025

arporter Feb 28, 2025

arporter Feb 17, 2025

arporter Feb 28, 2025

arporter Feb 17, 2025

arporter Feb 28, 2025

schreiberx commented Feb 20, 2025

arporter commented Feb 28, 2025

arporter left a comment

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025

arporter Feb 28, 2025


		return list(self._modules.values())

		def get_all_file_infos(self) -> List[FileInfo]:


		self._filepath_to_module_info[filepath] = module_info_in_file

		def get_all_module_infos(self) -> List[ModuleInfo]:

Martin fileinfo cache 2 #2833

Are you sure you want to change the base?

Martin fileinfo cache 2 #2833

Conversation

schreiberx commented Dec 19, 2024

schreiberx commented Dec 19, 2024

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

schreiberx commented Jan 10, 2025

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schreiberx commented Feb 20, 2025

arporter commented Feb 28, 2025

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 •

edited

Loading