Identifying routines internally by id instead of name #100

shamin-slac · 2024-10-02T21:25:57Z

Added an optional id field to the Routine class
When a new routine is saved, the id is created in the save_routine() method in db.py
Changed all database methods to reference routines by id instead of name
Updated the rest of the gui functionality accordingly
Updated tests accordingly
Now multiple routines can have the same name
If a routine has no runs, changes to the routine will simply update the routine
If a routine has runs and the only change is name and/or description, it will simply update the routine
If a routine has runs and there are changes that are not name and/or description, a new routine will be created

… ids - Update tests with ids

roussel-ryan

Looks good so far, I added some comments and I think we need some testing to make sure the ID behavior is as advertised.

roussel-ryan · 2024-10-02T21:31:12Z

src/badger/db.py


        con.commit()
        con.close()


 @maybe_create_routines_db
-def load_routine(name: str):
+def load_routine(id: str):


you said that the routine ID is optional? How is that handled in this function?

This function just takes an id, not a whole routine. In principle, the only place where there is a routine without an id is when a routine is composed from the routine editor (this is the main motivation to have id as an optional and not required field in the Routine class). Then in the save_routine() function, the id is generated and stored in that routine which is then saved in the database (and then it is subsequently retrieved for use in Badger). That being said, there are a couple places where I can imagine it's not impossible that there is no id, so I've modified the function to handle this possibility.

roussel-ryan · 2024-10-02T21:32:46Z

src/badger/db.py

@@ -20,7 +21,6 @@
    logger.info(
        f'Badger database root {BADGER_DB_ROOT} created')

-
 def maybe_create_routines_db(func):


I know this isn't a change from this PR but could be come up with a better name for this @zhe-slac @shamin-slac

Maybe ensure_routines_db_exists or require_routines_db?

so what is the purpose of this decorator?

To create the routines database if it doesn't already exist.

In that case I'm liking the ensure_routines_db_exists name and I would add a small docstring to the decorator that describes what it does

roussel-ryan · 2024-10-02T21:36:02Z

src/badger/tests/test_home_page.py

@@ -36,7 +36,7 @@ def test_home_page_run_routine(qtbot, init_multiprocessing):
    ]

    for ele in routines:
-        save_routine(ele)
+        ele.id = save_routine(ele)


can we do some additional testing here to make sure the ID generation is correct? Should also probably do a standalone file

roussel-ryan · 2024-10-02T21:39:49Z

src/badger/tests/test_core_subprocess.py

@@ -67,7 +67,7 @@ def test_run_routine_subprocess(

        self.routine = create_routine()
        time.sleep(1)
-        save_routine(self.routine)
+        self.routine.id = save_routine(self.routine)


should save_routine be setting the routine ID in place instead of what is done here?

Yeah that's my bad! I forgot that I was already setting the id of the routine object inside the save_routine() function itself haha. I've edited out these separate id assignments.

- Edit save_routine() to not output routine id and simply modify routine in place

- Add two tests for routine_id behavior

shamin-slac · 2024-10-18T17:04:17Z

Looks good so far, I added some comments and I think we need some testing to make sure the ID behavior is as advertised.

I've added some tests for the routine id behavior. It should be good to go, but let me know what you think.

roussel-ryan · 2024-10-23T20:44:46Z

@shamin-slac I tried running badger from your fork with a fresh install (which has an empty badger database I think) and ran into the following issue. I would suggest that you try to recreate this issue in your tests (which may involve clearing out the run database before running the tests) and then implement a fix to solve the issue

Traceback (most recent call last):
  File "/home/rroussel/miniforge3/envs/badger/bin/badger", line 8, in <module>
    sys.exit(main())
             ~~~~^^
  File "/home/rroussel/Badger/src/badger/__main__.py", line 108, in main
    args.func(args)
    ~~~~~~~~~^^^^^^
  File "/home/rroussel/Badger/src/badger/actions/__init__.py", line 15, in show_info
    launch_gui()
    ~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/__init__.py", line 75, in launch_gui
    window = BadgerMainWindow()
  File "/home/rroussel/Badger/src/badger/gui/default/windows/main_window.py", line 22, in __init__
    self.init_ui()
    ~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/windows/main_window.py", line 80, in init_ui
    self.home_page = BadgerHomePage(self.process_manager)
                     ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 93, in __init__
    self.init_ui()
    ~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 152, in init_ui
    self.refresh_routine_list()
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 390, in refresh_routine_list
    routine_ids, routine_names, timestamps, environments, descriptions = self.get_current_routines()
                                                                         ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 385, in get_current_routines
    routine_ids, routine_names, timestamps, environments, descriptions = list_routine(keyword, tags)
                                                                         ~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/rroussel/Badger/src/badger/db.py", line 37, in func_safe
    return func(*args, **kwargs)
  File "/home/rroussel/Badger/src/badger/db.py", line 196, in list_routine
    cur.execute(f'select id, name, config, savedAt from routine where name like "%{keyword}%" order by savedAt desc')
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.OperationalError: no such column: id

shamin-slac · 2024-10-23T20:56:02Z

@shamin-slac I tried running badger from your fork with a fresh install (which has an empty badger database I think) and ran into the following issue. I would suggest that you try to recreate this issue in your tests (which may involve clearing out the run database before running the tests) and then implement a fix to solve the issue

Traceback (most recent call last):
  File "/home/rroussel/miniforge3/envs/badger/bin/badger", line 8, in <module>
    sys.exit(main())
             ~~~~^^
  File "/home/rroussel/Badger/src/badger/__main__.py", line 108, in main
    args.func(args)
    ~~~~~~~~~^^^^^^
  File "/home/rroussel/Badger/src/badger/actions/__init__.py", line 15, in show_info
    launch_gui()
    ~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/__init__.py", line 75, in launch_gui
    window = BadgerMainWindow()
  File "/home/rroussel/Badger/src/badger/gui/default/windows/main_window.py", line 22, in __init__
    self.init_ui()
    ~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/windows/main_window.py", line 80, in init_ui
    self.home_page = BadgerHomePage(self.process_manager)
                     ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 93, in __init__
    self.init_ui()
    ~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 152, in init_ui
    self.refresh_routine_list()
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 390, in refresh_routine_list
    routine_ids, routine_names, timestamps, environments, descriptions = self.get_current_routines()
                                                                         ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/rroussel/Badger/src/badger/gui/default/pages/home_page.py", line 385, in get_current_routines
    routine_ids, routine_names, timestamps, environments, descriptions = list_routine(keyword, tags)
                                                                         ~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/rroussel/Badger/src/badger/db.py", line 37, in func_safe
    return func(*args, **kwargs)
  File "/home/rroussel/Badger/src/badger/db.py", line 196, in list_routine
    cur.execute(f'select id, name, config, savedAt from routine where name like "%{keyword}%" order by savedAt desc')
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.OperationalError: no such column: id

@roussel-ryan I think this is because you already have a routine table in the routines.db file in BADGER_DB_ROOT (which by default is ~/.local/share/Badger/db) from an old install of Badger, which is why it doesn't have the id column. So I would say this is a backwards compatibility issue.

roussel-ryan · 2024-10-24T17:38:45Z

Got it, thanks. Can we add some code to handle this and other issues when loading old routines? I think we discussed adding some kind of warning triangle to routines that failed to load in the GUI instead of raising errors during startup. We can also discuss this tomorrow

roussel-ryan

lgtm, thanks for the great work @shamin-slac

Shamin Chowdhury and others added 4 commits September 5, 2024 16:18

Update database methods and routine class to incorporate ids

e7bd6b9

- Implement ids beyond database functions, first functioning gui with…

189049d

… ids - Update tests with ids

Merge main branch

c1aa2b0

Fix routine saving so routine updates if no runs

4de8ad4

shamin-slac requested review from wenatuhs and roussel-ryan October 2, 2024 21:25

roussel-ryan reviewed Oct 2, 2024

View reviewed changes

shamin-slac added 4 commits October 8, 2024 08:34

Merge branch 'main' into id

8f40f0c

- Handle id that is not non-empty string in load_routine()

6761ecb

- Edit save_routine() to not output routine id and simply modify routine in place

- Delete runs automatically when routine is deleted

d4d7992

- Add two tests for routine_id behavior

- Add tests for modifying routines with runs

fdb45b6

Merge branch 'main' into id

ee68602

Rename the routines db and runs db decorators and add docstrings

c3a376b

shamin-slac and others added 2 commits October 28, 2024 09:11

Modify list_routine method to handle databases without routine ids

d31bac0

Merge branch 'xopt-org:main' into id

d799d25

roussel-ryan approved these changes Oct 28, 2024

View reviewed changes

roussel-ryan merged commit 6514fec into xopt-org:main Oct 28, 2024
5 checks passed

shamin-slac deleted the id branch October 28, 2024 17:21

roussel-ryan mentioned this pull request Oct 30, 2024

Allow users to change routine name under any conditions #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identifying routines internally by id instead of name #100

Identifying routines internally by id instead of name #100

shamin-slac commented Oct 2, 2024

roussel-ryan left a comment

roussel-ryan Oct 2, 2024

shamin-slac Oct 8, 2024

roussel-ryan Oct 2, 2024

shamin-slac Oct 8, 2024

roussel-ryan Oct 23, 2024

shamin-slac Oct 23, 2024

roussel-ryan Oct 23, 2024

roussel-ryan Oct 2, 2024

roussel-ryan Oct 2, 2024

shamin-slac Oct 8, 2024

shamin-slac commented Oct 18, 2024

roussel-ryan commented Oct 23, 2024 •

edited

Loading

shamin-slac commented Oct 23, 2024 •

edited

Loading

roussel-ryan commented Oct 24, 2024 •

edited

Loading

roussel-ryan left a comment

Identifying routines internally by id instead of name #100

Identifying routines internally by id instead of name #100

Conversation

shamin-slac commented Oct 2, 2024

roussel-ryan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamin-slac commented Oct 18, 2024

roussel-ryan commented Oct 23, 2024 • edited Loading

shamin-slac commented Oct 23, 2024 • edited Loading

roussel-ryan commented Oct 24, 2024 • edited Loading

roussel-ryan left a comment

Choose a reason for hiding this comment

roussel-ryan commented Oct 23, 2024 •

edited

Loading

shamin-slac commented Oct 23, 2024 •

edited

Loading

roussel-ryan commented Oct 24, 2024 •

edited

Loading