Skip to content

Conversation

rajeeja
Copy link
Contributor

@rajeeja rajeeja commented Jun 12, 2023

Remove specific version dependency on protobuf, the old problems seems to be fixed in newer versions of protobuf. Tested on OSX.

Remove specific version dependency on protobuf, the old problems seems to be fixed in newer versions of protobuf. Tested on OSX.
@rajeeja rajeeja requested a review from j-woz June 12, 2023 23:35
@j-woz
Copy link

j-woz commented Jun 13, 2023

This works for me.

@rajeeja
Copy link
Contributor Author

rajeeja commented Jun 22, 2023

@jmohdyusof merge?

@jmohdyusof
Copy link
Collaborator

You need to test actual benchmarks. Almost all the benchmarks in Pilot1 and Pilot3 fail with:
TypeError: Descriptors cannot not be created directly.
If this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:

  1. Downgrade the protobuf package to 3.20.x or lower.
  2. Set PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python (but this will use pure-Python parsing and will be much slower).

All the data files need to recreated with newer protobuf, based on that error message.

@rajeeja
Copy link
Contributor Author

rajeeja commented Jun 23, 2023

All the data files need to recreated with newer protobuf, based on that error message.
How to resolve dependency issues? it is a nighmare to reolve dependency issues on new HPC machines

Option 1:
I think we should coordinate with model owners and discuss the situation with them. Request that they provide the scripts or tools they used to generate the data files. This way, you can recreate the data files using the newer version of protobuf while ensuring compatibility with the benchmarks. This is a hard option, but, doable.

Option 2:
Create a separate branch: If it's not feasible to update the benchmarks immediately, you can create a separate branch in the candle_lib repository specifically for supporting the benchmarks in the Benchmarks repository. This branch, let's call it benchmarks_compatible, can be dedicated to maintaining compatibility with the older version of protobuf required by the benchmarks. This approach allows you to freeze the necessary codebase for the benchmarks while allowing the master branch to move forward with updates and improvements.

Another key issue:
Plan for a new release: @brettin what is the timeline to determine a timeline for a new release of candle_lib? This new release should incorporate updates, bug fixes, and improvements while considering compatibility requirements with the benchmarks. Discuss the possibility of diverging from the notion of the master branch supporting all benchmarks and confining it to the benchmarks_compatible branch.

What ever route we follow, we should communicate the plan, decisions, and any necessary changes to all relevant stakeholders. This documentation will help maintain clarity and facilitate future development and collaboration.

@jmohdyusof
Copy link
Collaborator

Well, a lot of these are 'your' benchmarks, so you can update them is you wish. All the Pilot1 benchmarks fail

@rajeeja
Copy link
Contributor Author

rajeeja commented Jun 23, 2023

Well, a lot of these are 'your' benchmarks, so you can update them is you wish. All the Pilot1 benchmarks fail

I didn't write the benchmarks and don't have the scripts. Let's go with Option2.

@jmohdyusof
Copy link
Collaborator

What do you mean 'I don't have the scripts'? They are all there in Pilot 1

@rajeeja
Copy link
Contributor Author

rajeeja commented Jun 23, 2023

What do you mean 'I don't have the scripts'? They are all there in Pilot 1

scripts that were used to generate the input data files.

@jmohdyusof
Copy link
Collaborator

I assume Tom, Fangfang, Yitan must have these? I expect if they are rerun with a newer protobuf environment they would work?

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.

3 participants