Fix a date-time bug; drop pointless dependencies; and deprecate utils. by dmyersturnbull · Pull Request #85 · rcsb/py-rcsb_db

dmyersturnbull · 2025-02-04T00:22:20Z

@piehld Another PR will remove MySQL. This PR fixes a bug and removes some unnecessary dependencies.

Summary: Fixes a date-time calculation bug; drops pointless date-time dependencies; and deprecates TimeUtil and TextUtil.

TimeUtil incorrectly called datetime.replace(tzinfo=utc), which does doesn't actually move timezones. Code using TimeUtil and useUtc was affected.
Drops the dependencies dateutil, pytz, and strict-rfc3339. Builtin datetime works perfectly well. Removes these from TimeUtil and some other code.
Refactored code to not use TimeUtil. Its methods just delegate to datetime in standard ways. [1]
Cleaned up TextUtil and refactored code to avoid using it. It only defined unescapeXmlCharRef, which is just html.unescape.
Deleted unescape.py, which was an unused duplicate of TextUtil. [2]
Dropped the dependencies scandir and configparser, which were marked ; python<3.0.
Dropped the dependencies six and future, removed from __future__ import generators, and moved jsondiff to tests_require. (2nd commit)

[1] The exception is "week signature" because it's non-obvious/nonstandard (i.e. not ISO 8601 week number). However, it's just strftime("%Y_%V").
[2] This is technically a breaking change, but adding import rcsb.db.utils.TextUtil as unescape to utils.__init__.py would make it non-breaking.

piehld

Thank @dmyersturnbull, definitely appreciate you reviewing that codebase and identifying and proposing fixes to outdated code. I'm sure your changes are reliable and will try to review them soon, but because I'm out the rest of the week, I just want to give you a heads up that we will probably wait until next week to merge this, since I want to be extra careful testing this before pushing out to production for the weekend. Any changes with rcsb.db are pretty impactful with the weekly-update, so I just tend to lean towards extra caution when it comes to pushing things out is all.

While I still need to go through each change, the one thing that stands out to me is the removal of the mock-data submodule. I'm guessing that wasn't intentional...can you add it back in there?

Also FYI @brindakv

dmyersturnbull · 2025-02-04T18:06:16Z

I want to be extra careful testing this before pushing out to production for the weekend.

Sounds good. The SQL removal doesn't depend on this.

By the way, do tests run for PRs?

While I still need to go through each change, the one thing that stands out to me is the removal of the mock-data submodule. I'm guessing that wasn't intentional...can you add it back in there?

Ah! Git doesn't normally track empty directories, and I had to rmdir to bypass an error. Apparently that deletes submodules. I'll fix it.

piehld · 2025-02-04T21:53:23Z

Thanks! And be aware that Michael just merged a PR, so you'll probably want to rebase or merge.

By the way, do tests run for PRs?

Yes, they do. Every time you push a new commit a new pipeline will run. However, for some reason this repository doesn't show the results from the runs like other repos do, so you'll need to just go to Azure to check the run directly: https://dev.azure.com/rcsb/RCSB%20PDB%20Python%20Projects/_build?definitionId=33

When you push a new commit, a new run will show up on that link.

…xtUtil

� Conflicts: � rcsb/db/cockroach/CockroachDbUtil.py � rcsb/db/cockroach/Connection.py � rcsb/db/crate/CrateDbUtil.py � rcsb/db/mysql/MyDbUtil.py � requirements.txt � setup.py

dmyersturnbull · 2025-02-14T23:45:12Z

@piehld I rebased. I also adjusted the readme (no longer mentions MySQL) and cleaned up setup.py. I moved rcsb.utils.chemref to the test requirements.

piehld

Thanks for bringing mock-data back @dmyersturnbull, and the other changes! I have a couple general questions about the removal of TimeUtil.py, which I left as comments for that file. I definitely agree with removing unnecessary dependencies from requirements.txt, but do wonder if we need to remove that file altogether or instead just update it, since that class is used in code elsewhere that would need to be updated also if we remove it. So, personally I'd kind of prefer keeping the class but just updating it to use datetime instead.

piehld · 2025-02-24T19:01:24Z

rcsb/db/utils/TimeUtil.py

This class is used in rcsb.exdb and rcsb.workflow, so we would need appropriate updates to those repos too (and first) if we want to get rid of this file.

Alternatively, instead of getting rid of this file (since it is used in a handful of places), is it possible to just update this file to not use dateutil or pytz? That would be my preference, at least.

This class is used in rcsb.exdb and rcsb.workflow

I don't think that's true. rcsb.exdb is importing rcsb.utils.io.TimeUtil (which, incidentally, is identical). The first commit in this PR refactored TimeUtil, but I deleted it when I realized that.

Hmm, I noticed that difference too, but I also see imports of rcsb.db.utils.TimeUtil, e.g.:

https://github.com/rcsb/py-rcsb_exdb/blob/master/rcsb/exdb/utils/ObjectTransformer.py#L20

https://github.com/rcsb/py-rcsb_exdb/blob/master/rcsb/exdb/utils/ObjectValidator.py#L24

https://github.com/rcsb/py-rcsb_workflow/blob/master/rcsb/workflow/wuw/ExDbWorkflow.py#L23

Oh, good catch. I added stubs to rcsb/db/utils/__init__.py that provide those with deprecation warnings.

piehld · 2025-02-24T20:51:49Z

rcsb/db/processors/DataExchangeStatus.py

+from datetime import datetime
+from typing import Optional
+from zoneinfo import ZoneInfo


Likewise, if we keep TimeUtil, we don't have to worry about updating each of these files that currently imports it.

piehld · 2025-02-24T20:57:02Z

rcsb/db/processors/DataTransformFactory.py

+        # TODO
+        # What is this function doing???!
+        # Its name was castDateTimeToIsoDate, suggesting it outputs YYYY-MM-DD.
+        # Contradicting tht, the docstring said:
+        # > Cast the input date (optional time) string (yyyy-mm-dd:hh::mm:ss) to a Python DateTime object
+        # Instead, it did this:
+        # > tS = dateutil.parser.parse(tv).replace(tzinfo=pytz.UTC).isoformat()
+        # That's three contradictory claims.
+        # What was this function supposed to do?
+        # Why did it fill in 00:00:00, and why did it append +00:00?
+        # Also, why did the format separate the date and time with `:`?
+        # END TODO


lol, these are all good questions. I imagine some of the contradictions probably came from John copying and pasting a docstring or changing the method name after already writing an [outdated] docstring. From experience, he wasn't always super careful with docstrings.

So, I think the best way to figure out what the intent of this was would be to run an actual load and add some logging to figure it out. I could look into doing that.

I simply retained the current behavior and ignored the intention (save for those comments).

dmyersturnbull requested a review from piehld February 4, 2025 00:22

piehld requested changes Feb 4, 2025

View reviewed changes

dmyersturnbull added 3 commits February 14, 2025 15:00

fix: correct incorrect timezone conversion; deprecate TimeUtil and Te…

8ee63be

…xtUtil

fix: remove dependencies six, future, and jsondiff

87c6e98

� Conflicts: � rcsb/db/cockroach/CockroachDbUtil.py � rcsb/db/cockroach/Connection.py � rcsb/db/crate/CrateDbUtil.py � rcsb/db/mysql/MyDbUtil.py � requirements.txt � setup.py

drop: delete TextUtil and TimeUtil

c6f228d

dmyersturnbull force-pushed the dev-dmt-fix-timestamp branch from e3bc77c to c6f228d Compare February 14, 2025 23:40

dmyersturnbull added 3 commits February 20, 2025 18:11

fix: lint rule violation

9af75d1

ci: fix linting inconsistency

7009c11

style: remove useless code

18dcff2

dmyersturnbull force-pushed the dev-dmt-fix-timestamp branch from 8e67cee to 18dcff2 Compare February 22, 2025 00:19

piehld reviewed Feb 24, 2025

View reviewed changes

dmyersturnbull added 2 commits February 26, 2025 17:03

fix: add deprecated imports

64427d2

fix: correct buggy usage of datetime and time

4664820

Conversation

dmyersturnbull commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piehld left a comment

Choose a reason for hiding this comment

Uh oh!

dmyersturnbull commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piehld commented Feb 4, 2025

Uh oh!

dmyersturnbull commented Feb 14, 2025

Uh oh!

piehld left a comment

Choose a reason for hiding this comment

Uh oh!

piehld Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

piehld Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

dmyersturnbull Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piehld Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

dmyersturnbull Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piehld Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

piehld Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

dmyersturnbull Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmyersturnbull commented Feb 4, 2025 •

edited

Loading

dmyersturnbull commented Feb 4, 2025 •

edited

Loading

dmyersturnbull Feb 24, 2025 •

edited

Loading

dmyersturnbull Feb 27, 2025 •

edited

Loading