Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Pbp job agent #47

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

spacetimeengineer
Copy link
Contributor

I am sending this PR as a formality after our last demo. I am not sure how to set it up as a branch on your repo. I developed a job agent utility for SOUNDTRAP and NRS, incorporating new datetime formats and regex patterns. Most of the code is new and doesn't interfere with your existing work. I only took the liberty of modifying the Windows-based routines in your code base which I improved upon by utilizing the Path library in places where it belonged.

Will test until it works!
Added test yamls
Good stopping point.
end of day commit
End of day commit
Job agent seems to work on linux
pbp-job-agent seems to be working!
Added some extra log stuff
Improved argument structure
Added multiprocessing
Multiprocessing works on windows and linux.
Fixed some logging.
Added comments
Ran a ruff format on job agent
Ran another ruff
Made some fixes picked up by CI
Fixed even more errors picked up by CI
A bug of extra stuff and some improved test structuring.
Ruff
Attempt to fix merge
Attempt to fix merge
Small updates
Added Samaras Files
Got NRS with job agent partially working
NRS is compatible with the pbp-job-agent.
updated files
Attempting to fix logs
So I cant use a non global logger so I just re-added the logger in the run().
This was a testing artifact.
Ran a ruff format pbp
fixed sountrap issue
fixed sountrap issue
Fixed things using the path library more often.
@carueda
Copy link
Member

carueda commented Dec 16, 2024

Hi @spacetimeengineer - Seems like there's a failed check.

More in general, when do you think you will be completing your changes (and providing instructions) so @danellecline , @ryjombari and I can plan on reviewing/testing? If possible, we would like to advance that review exercise in preparation for our meeting this Thursday. Thanks!

@carueda
Copy link
Member

carueda commented Dec 16, 2024

@spacetimeengineer – I checked out your branch in my local dev env.

  • all looking good to me so far. As you say, all is pretty much new code with no, or very minimal adjustments in pre-existing code.

  • seems like a file needs reformatting (you can run just format)

  • consistent with CI, there's a failed test:

tests/test_file_helper.py .
tests/test_json_support.py ...
tests/test_meta_generator.py .F...
tests/test_metadata.py ...
tests/test_misc.py ..
tests/test_simpleapi.py ..
========== FAILURES ==========
___ test_soundtrap_generator_local _____________
...
>       assert len(json_files) == 1
E       assert 0 == 1
E        +  where 0 = len([])

tests/test_meta_generator.py:129: AssertionError
----------------------------------------------------------------------- Captured stderr call ------------------------------------------------------------------------
2024-12-16T15:08:20.894095-0800 INFO Coverage plot saved to tests/json_generator_tmp/soundtrap_s3/soundtrap_coverage_20230715_20230716.jpg
2024-12-16T15:11:42.895969-0800 INFO Searching in file:///Users/carueda/github/mbari-org/soundscape-repos/pbp/tests/wav/soundtrap/*.wav for wav files that match the prefixes ['6716']* ...
Searching : / |#                                      | 0 Elapsed Time: 0:00:00
Searching : - |#                                      | 0 Elapsed Time: 0:00:00
2024-12-16T15:11:42.899441-0800 INFO Found 0 files to process that covers the expanded period 2022-11-15 00:00:00 - 2022-11-17 00:00:00
2024-12-16T15:11:42.899736-0800 INFO No data found between 2022-11-16 00:00:00 and 2022-11-16 00:00:00

Copy link
Member

@carueda carueda left a comment

Choose a reason for hiding this comment

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

(just making github happy) I indicated some initial changes in my previous comment.

@carueda
Copy link
Member

carueda commented Dec 17, 2024

@spacetimeengineer – We have just updated the main branch with incorporation of the end-user documentation setup and markdown sources (which were previously maintained in a separate repo).
You will see a new pbp-doc/ directory when you sync your branch.

It would be great if you could also add some documentation for the new job agent in that setup, which is based on mkdocs, with mkdocs-material. In a nutshell, under pbp-doc/, it would be a matter of:

  • creating a pbp-job-agent/ dir and adding an index.md file there
  • under the nav section in mkdocs.yml, adding something like:
      - Job Agent: pbp-job-agent/index.md
  • adding documentation in pbp-job-agent/index.md

Thanks for considering.

@carueda carueda marked this pull request as draft December 18, 2024 17:20
Attempting a fix and to debug Nonetype error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants