integration of host system information script into Nominatim CLI tool #2739

tareqpi · 2022-06-07T09:00:33Z

Refactored and included additional host system information into collect_os_info.py
Integrated collect_os_info.py into Nominatim's CLI tool
Added a test for the new feature in the CI flow

This pull request resolves #2552 and #307

lonvia

I've just added some minor comments. Looks okay in general. pylint has some more complaints, see the CI. You may need to install the latest version of pylint from pip to get the same reports locally.

The file collect_so_info.sh can be deleted, I think?

Could you please add a call to your new command in the CI run, around here. Simply duplicate what is done for the version call. That way, we can see in the CI directly what the output looks like.

lonvia · 2022-06-07T15:07:56Z

nominatim/clicmd/admin.py

@@ -58,16 +61,23 @@ def run(args):
            LOG.warning('Analysing performance of indexing function')
            from ..tools import admin
            with connect(args.config.get_libpq_dsn()) as conn:
-                admin.analyse_indexing(conn, osm_id=args.osm_id, place_id=args.place_id)
+                admin.analyse_indexing(
+                    conn, osm_id=args.osm_id, place_id=args.place_id)


Please do not reformat unrelated code.

lonvia · 2022-06-07T15:09:04Z

nominatim/tools/collect_os_info.py

+        message = """
+Use this information in your issue report at https://github.com/osm-search/Nominatim/issues
+Copy and paste or redirect the output of the file:
+    $ ./collect_os_info.py > report.md


Needs adaption to the new command.

lonvia · 2022-06-07T15:11:20Z

nominatim/tools/collect_os_info.py

+        self.os_info: str = self._os_name_info()
+
+        self.nominatim_ver: str = '{0[0]}.{0[1]}.{0[2]}-{0[3]}'.format(
+            NOMINATIM_VERSION)


nominatim.version now has a convenience function version_str().

I globally searched this function and couldn't find it.

I mean this one here:

Nominatim/nominatim/version.py

Line 40 in 8002405

def version_str(version=NOMINATIM_VERSION):

Yes, I have synced up the branch to the master branch because the branch was not updated for a long time and then found that function.

lonvia · 2022-06-14T18:48:00Z

nominatim/tools/collect_os_info.py

+    """Generate a report about the host system including software versions, memory,
+    storage, and database configuration."""
+
+    with connect(config.get_libpq_dsn()) as conn:


The CI test highlights a problem here: this will only work when a Nominatim database was already imported. To avoid that issue, try to connect against database 'postgres' instead. That will always exist.

In general, I would prefer that we catch any exception raised during connect and report something like 'Unable to connect to database' where the version string would be reported.

To clarify, to connect to the postgres database you can simply use connect("dbname=postgres")

lonvia · 2022-08-05T07:52:32Z

nominatim/tools/collect_os_info.py

+        with conn.cursor() as cur:
+            nominatim_db_exists = cur.execute("""
+            SELECT datname FROM pg_catalog.pg_database 
+            WHERE datname='nominatim'""")


You cannot really make the assumption that the database is always called 'nominatim'. Users can use arbitrary names in their configuration.

That said, we cannot even make the assumption that they are using a local database. So I was wrong when recommending connect("dbname=postgres"). We need to use any other connection parameters from the user-supplied dsn. You can use make_dsn() to replace the database name in the DSN that you get from config.get_libpq_dsn():

connect(psycopg2.make_dsn(config.get_libpq_dsn(), dbname='postgres'))

You can also use parse_dsn to extract the database name from the DSN and then use the SELECT to find out if the database exists.

lonvia · 2022-08-05T07:54:31Z

nominatim/tools/collect_os_info.py

@@ -0,0 +1,165 @@
+"""


Please add a license header here (simply copy from other .py files).

tareqpi · 2022-08-06T23:06:18Z

Some of the third-party libraries' functions that are introduced in this pull request lack the correct type hints which is why mypy produces errors. I have raised an issue to them and made the necessary pull requests to resolve it as well.

lonvia · 2022-08-07T08:07:02Z

I've usually marked the calls with # type: ignore[no-untyped-call], see for example

Nominatim/nominatim/db/connection.py

Line 34 in eecc73e

    
           LOG.debug(self.mogrify(query, args).decode('utf-8')) # type: ignore[no-untyped-call]

But if you contribute the necessary typing info to typeshed, even better.

lonvia · 2022-08-08T07:50:10Z

With your typeshed PR merged (jay!), there was a new mypy error for a now unneded ignore[no-untyped-call]. I've already fixed that one master. Please rebase.

lonvia

Can you please rebase this once more on master. #2792 already had the adaptions for the type annotations. Also consider squashing your commits to get rid of the intermediate steps. If you do that, only squash your commits, not the ones from micahcochran.

lonvia · 2022-08-11T07:28:29Z

nominatim/tools/collect_os_info.py

+
+    try:
+        with open("report.md", "w", encoding='utf8') as output:
+            output.write(report)


I would prefer if it just writes to stdout, i.e. uses print. Users can always redirect to a file if they want it in a file. If left like this we'd have to add a parameter to state the output filename and that makes things more complicated.

Changed the script to functional programming paradigm to remove the big number of local attributes to decrease memory usage when running it. Additional OS info are now included.

micahcochran added 2 commits December 13, 2021 11:26

add utils/collect_os_info.sh script

f20d857

made collect_os_info script in Python

8bda59f

lonvia reviewed Jun 7, 2022

View reviewed changes

lonvia reviewed Jun 14, 2022

View reviewed changes

lonvia reviewed Aug 5, 2022

View reviewed changes

nominatim/tools/collect_os_info.py

@@ -0,0 +1,165 @@

"""

Copy link

Member

lonvia Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a license header here (simply copy from other .py files).

lonvia reviewed Aug 11, 2022

View reviewed changes

tareqpi force-pushed the collect_os_info.sh branch 5 times, most recently from aa71002 to 72ac916 Compare August 12, 2022 20:50

Merge remote-tracking branch 'upstream/master' into collect_os_info.sh

5e477e3

tareqpi force-pushed the collect_os_info.sh branch from 6d60306 to 5e477e3 Compare August 12, 2022 21:55

tareqpi added 3 commits August 13, 2022 06:13

Enhanced and refactored 'collect_os_info.py'

49f889b

Changed the script to functional programming paradigm to remove the big number of local attributes to decrease memory usage when running it. Additional OS info are now included.

Integrated 'collect_os_info.py' into Nominatim's CLI tool

465d82a

Added the feature of collecting host system information to the CI tests

7401987

lonvia merged commit 07d72f9 into osm-search:master Aug 13, 2022

lonvia mentioned this pull request Nov 16, 2022

debug script which shows all OS,Postgres,Nominatim parameters #307

Closed

Uh oh!

integration of host system information script into Nominatim CLI tool #2739

integration of host system information script into Nominatim CLI tool #2739

Uh oh!

Conversation

tareqpi commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonvia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tareqpi commented Aug 6, 2022

Uh oh!

lonvia commented Aug 7, 2022

Uh oh!

lonvia commented Aug 8, 2022

Uh oh!

lonvia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tareqpi commented Jun 7, 2022 •

edited

Loading