-
Notifications
You must be signed in to change notification settings - Fork 24
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: 7net-mf-ompa, 7net-omat #184
Conversation
sevenn/util.py
Outdated
except ValueError: | ||
raise ValueError( | ||
f'Given {checkpoint} is not exists and not a pre-trained name' | ||
) | ||
return SevenNetCheckpoint(checkpoint_path) | ||
|
||
|
||
def download_checkpoint(checkpoint_name: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수 pretrained name to path 내부로 이동
subprocess run이나 os system은 반드시 그걸 써야만 할 때 써야함. 대응되는 라이브러리 있으니 그걸 사용 (mace 참고)
패키지 위치에 write권한이 없는 경우 에러날 것. 패키지 위치에 먼저 시도하고, 쓰기 권한 없으면 HOME/.cache 아래에 쓰기. (os 모듈 써서 path expand 시켜야 다른 플랫폼에서도 동작함 참고)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다운로드 받는 부분 에러나기 쉬우니 try except로 감싸고 적절한 에러 메시지 띄우셈
- 가능하면 url 하드코딩 하지말고 _const.py로 이동
Kappa 전용 모델도 공개하기로 했으니 그것도 부탁해 체크리스트 (필요시 추가)
|
sevenn/util.py
Outdated
save_path = home_save_path | ||
os.makedirs(save_path, exist_ok=True) | ||
checkpoint_path = checkpoint_path2 | ||
except ValueError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Home 은 write 권한 있으므로 불필요함. 이 경우 permission error지 value error 를 띄우지 않음.
sevenn/util.py
Outdated
f'Failed to create save path for {model_name} checkpoint' | ||
) | ||
print(f'Saving to {save_path}', flush=True) | ||
with tempfile.NamedTemporaryFile(delete=False, dir=save_path) as temp_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내 생각에는 쓰기 권한 있는 path를 확보했으니 temp 파일을 쓸 필요가 없어보이는데 혹시 어떤 이유가 있음?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urllib으로 다운받다가 에러나서 중단되면 checkpoint_~.pth가 일부만 다운받아진 상태로 파일로 남게돼서 다음 코드 실행시 경로에는 체크포인트가 있다고 인식하는데 미완전한 체크포인트 파일이라 에러가 떠
그래서 temp 형식으로 다운받게 해서 완전히 다운로드 성공할 경우 이름을 바꾸는 식으로 했어
sevenn/util.py
Outdated
KeyboardInterrupt, | ||
) as e: | ||
raise ValueError(f'Failed to download {model_name} checkpoint: {e}') | ||
finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 finally의 목적은?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temp 형식으로 다운받다가 중간에 중단되면 미완전한 temp 파일이 그대로 남아서 해당 경로에 더미파일이 남게 돼. 더미 파일을 없애는 용도로 finally에다가 미완료된 temp파일을 지워버리도록 코드를 작성했어
이거 다운로드 너무 느리네 ㅋㅋㅋㅋ 용량을 좀 줄여야 하는데 기존 체크포인트에서 저렇게 업데이트된 체크포인트로 테스트 해보고 figshare말고 bandwidth 좀 빠르게 공유할 수 있는 곳 혹시 있는지 찾아봐 줄 수 있어? 답변 준 부분들은 무슨 이슈인지 이해함 |
체크포인트 용량 ompa: 99mb / omat: 61mb로 줄었고 다운시간은 그때그때 다르지만 길어도 1분안에는 다 다운 되는듯. |
sorry for the unsolicited comment. just wanted to say congrats 🎉 since you guys currently have the best model submitted to matbench discovery according to the new CPS metric introduced in janosh/matbench-discovery#223. that's no small feat given the tough competition. 😄 |
@janosh thanks for the news! The CPS graph looks really great 👍 If matbench is going to add new metrics and you need data from 7net, please send me an e-mail and I'm happy to help as I appreciate your effort in the leaderboard active. Thanks! |
No description provided.