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

Bugs in Notebook #1

Open
vsoch opened this issue Mar 15, 2023 · 8 comments
Open

Bugs in Notebook #1

vsoch opened this issue Mar 15, 2023 · 8 comments

Comments

@vsoch
Copy link

vsoch commented Mar 15, 2023

Hiya! I was able to get everything running with maestro in a container, and I added a final step to dynamically render the notebook, and ran into a few bugs I'd like to report / discuss here.

The first was an issue with a pydv import - I opened an issue and PR to fix it, and my workaround for the time being is to use a fixed version of the module. LLNL/PyDV#276

The second was that the group name was hard coded - I decided instead to just choose the first from the set:

# Second issue - hard coded group
# For the purposes of this demo, chose the first.
GROUP_OF_INTEREST = list(groups)[0]
print(f"Group of interest is {GROUP_OF_INTEREST}")

The third was that the plotting seems to be outdated (or I suspect the TODO suggests it wasn't implemented yet?) "curves" is a list, and it doesn't have a function create_curve so I commented it out:

# Commented out - this returns a list and does not work
# Likely it's raw data we just need to plot with matplotlib
curves = pydv.readsina(PYDV_DEMO_FILENAME)  # raw?
print(curves)
# curve = curves.create_curve(x="time", y="y_pos")
# plt.plot(pydv.fft(curve))

Finally (this one I didn't dive into debugging) there is a key error:

KeyError                                  Traceback (most recent call last)
Cell In[1], line 23
     21 datastore = create_datastore(database)
     22 recs = datastore.records
---> 23 vis = Visualizer(datastore)
     24 print("Sina is ready!")

File /usr/local/lib/python3.8/dist-packages/sina/visualization.py:86, in Visualizer.__init__(self, ds, id_pool, matplotlib_options)
     84 self.data_names = self.get_contained_data_names()
     85 # Used for histograms and the like
---> 86 self.data_names["scalar_and_string"] = sorted(set(self.data_names["scalar"])
     87                                               .union(self.data_names["string"]))
     88 self.id_pool = id_pool
     89 self.matplotlib_options = matplotlib_options

KeyError: 'scalar'
KeyError: 'scalar'

Help with the above would be great! The container I have as a WIP (pending these fixes) is here: rse-ops/flux-hpc#23. Once these updates are in it should be fairly trivial to merge, that will trigger an automated build to GitHub packages, and then I can throw together a test recipe to see how Maestro performs on the flux operator. My one concern is that Maestro doesn't have a single run command that will continue (and not exit) while everything is running, so we will need to use an interactive submit, and then wait for the output files to appear as an indication that everything is done. If you know maestro, I also couldn't figure out how to use matestro status (sorry was my first time!)

Thanks for the help!

@doutriaux1
Copy link
Contributor

Thanks @vsoch I believe some of these are fixed in some other branches.

@doutriaux1
Copy link
Contributor

@vsoch also please take a look at the trilab-demo branch it's a bit more up to date.

@HaluskaR
Copy link
Contributor

Great catch on the last one! That KeyError: 'scalar' is caused by the visualizer expecting there to be scalar data, but finding none. Most of the time this is unintentional (setting up a visualizer against an empty database) since most dbs have some scalar data in them. That said, absolutely shouldn't throw an error, should print a warning at most. I'll get on it!

@vsoch
Copy link
Author

vsoch commented Mar 15, 2023

Thanks @doutriaux1 and @HaluskaR - are there plans to merge the trilabs branch into main so folks can easily use the latest?

@doutriaux1
Copy link
Contributor

@vsoch yes we will be merging a few branches soon (on gitlab) and bring it back here.

@vsoch
Copy link
Author

vsoch commented Apr 5, 2023

heyo! Wanted to circle back and see if there are any updates? I can add the current (partial workflow) as an example to the Flux Operator, but I'd love to have the whole thing working fully as expected. Thank you!

@HaluskaR
Copy link
Contributor

HaluskaR commented Apr 5, 2023

Fix for the last one's in! https://lc.llnl.gov/gitlab/siboka/Sina/-/merge_requests/219

This'll replace the ValueError a warning, but I did discover that the path causing the error is for a truly empty database. Given that the snippet appears to be connecting to an existing database, it might be worth double-checking that path.

@vsoch
Copy link
Author

vsoch commented Apr 5, 2023

Does the existing database get generated by the first notebook of the tutorial (and thus this is the path I should check) is it expected to be provided by the knowledgeable user?

If the first - I can double check my path!

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

No branches or pull requests

3 participants